Move coinbase & coinstake to P2PKH #1700

furszy · 2020-06-20T17:52:12Z

Moved the last remaining P2PK scripts to P2PKH (coinbase and the staking output to the current P2PKH standard).

The main argument used for the migration is that we are already using it as a standard everywhere in the sources (same as upstream) and has an increased, theorical, security due the pub key not being revealed initially (scriptsig reveals it).

random-zebra · 2020-06-22T15:58:57Z

This is a consensus change.
If not properly handled with a network upgrade, I think it will have nasty consequences, probably forking the chain.

A client updated with this code, staking a P2PKH utxo, will produce a P2PKH coinstake output, and old clients will not be able to verify the block signature (they will try to get the public key from the coinstake output, which in this case will have only the pubkey hash).
New clients, on the other hand, being able to grab the public key from the coinstake input scriptSig, will be able to verify the block signature and accept the block.
This might cause a chain split, or, at least, make it so new clients will be unable to stake "new" utxos (they will be able to stake only the outputs of previous coinstakes, as those are P2PK and remain so).

Further, enforcing scriptPubKey = scriptPubKeyKernel for all scripts, is another consensus change.
An old-client staker might theoretically send the stake to a P2PK script different from the one being spent, and use the corresponding key to sign a block. New clients will verify the block signature with a wrong public key (the one taken from the input), thus failing CheckBlockSignature and rejecting the block, while the peers running the old code will accept it.

That said, I agree conceptually with the change (although it'll have to wait for 5.0) for the space savings (the privacy/security gain is kinda pointless, as we are spending the same script in the same tx, thus revealing the public key since the creation of the new output anyway).

furszy · 2020-06-22T17:27:43Z

yep 👍 , for 5.0 will be.

furszy · 2020-07-15T01:45:43Z

Updated it with the v5 upgrade check.

random-zebra

It seems that the upgrade logic has been enforced with a block version check.
But the CBlock::CURRENT_VERSION is not bumped here and it sits at 7 (even after the network upgrade).

The variable onlyP2PK , defined in CreateCoinStake, depends on the chain height (and not on the block version). So, with this code, after the v5 NU, clients would create blocks with P2PKH coinstakes, but still with version 7, and thus those would be rejected by other peers because CheckBlockSignature would fail.

That said, the block version, alone, is not enough to ensure that the new rules apply only after the enforcement: consensus does not prevent block versions higher than CURRENT_VERSION, it only rejects outdated versions (i.e. it is possible to stake v8 blocks, before the v5 network upgrade… it is NOT possible to stake v7 blocks after… see ContextualCheckBlockHeader).

Also, I know that, with 5.0, we will have to bump the block version due to other reasons… but this particular change does not require it (as nothing changes in the block structure or serialization): all the logic could be handled simply checking the chain height (both on block creation and on block verification).

Finally, given the risks involved, this needs a functional test.

furszy · 2020-07-17T15:28:40Z

It seems that the upgrade logic has been enforced with a block version check.
But the CBlock::CURRENT_VERSION is not bumped here and it sits at 7 (even after the network upgrade).

The variable onlyP2PK , defined in CreateCoinStake, depends on the chain height (and not on the block version). So, with this code, after the v5 NU, clients would create blocks with P2PKH coinstakes, but still with version 7, and thus those would be rejected by other peers because CheckBlockSignature would fail.

Yeah ok, this PR was another extraction from the Sapling work. The block version bump is due the new Sapling notes merkle tree hash stored on the header. It's not here, neither we are enforcing block v8 at all on this PR. It's done later on the Sapling work.

Will add the enforcement height check. The idea there was try to prevent the height check as it makes the code ugly. In other scenarios (if we wouldn't be doing a hardfork for Sapling anyway), we could had made this upgrade as a soft fork.

About the test, we spoke about it during the last meeting. Currently, the miner unit tests are not connected and our miner code needs a major refactoring (adapting the BlockAssembler class and cleaning up the structure -- btc 7598 path --). Not a straightforward change there to get into this PR.
What can do is add a functional test for now but.. damn that it will just be another slow test.

furszy · 2020-07-17T15:54:38Z

Actually, there are other issues making the height enforcement change.

As the code is now, we cannot add the enforcement height check there. CheckBlockSignature is called at the beginning of ProcessNewBlock and is processed without tip contextual data (up until that point, we don't know the height of the block, nor if the previous block is already on disk -- if it's not on disk, it will not be processed and the node will request sync from the previous one first).

Will think more about this topic. We shouldn't move the signature check to be a tip contextual check, it needs to be done before even try to look for the previous block on disk to detect fake blocks/chains and not request them.

furszy · 2020-07-20T16:15:48Z

PR updated with an unit test covering the pre/post enforcement block signature verification, added documentation in checkBlockSignature function and a functional test validating the chain movement after the upgrade activation.

furszy · 2020-07-20T16:19:15Z

as an extra note, have rebased the PR on top of #1747 to not duplicate the v5 activation height set for regtest.
We need to merge that one first, then will rebase this one again.

random-zebra

Looking good now, thanks for the tests. 👍
Will give final ACK after the rebase.

furszy · 2020-07-22T18:12:04Z

done, rebased

random-zebra

tested ACK 3a27245

Fuzzbawls

ACK 3a27245

with just two minor, non-blocking nits that can be dealt with later

src/blocksignature.cpp

src/stakeinput.cpp

rpc_blockchain.py: decreased the amount of bytes

For some reason, `GetKeyFromPool` could return an already used key. Most likely has to do with the way that we are creating blocks in regtest. Before, the issue was not popping up because we were using P2PK in the coinbase and coinstake. And in P2PK scripts even when the pubkey was already used, the used/unused key check only checks for P2PKH scripts.

…5 upgrade. Only P2PK and P2CS outputs were accepted before block version 8 due a badly implemented block signature check.

If the staking tx has a P2PK scriptsig input and a P2PKH scriptpubkey output, there is no way to obtain the public key from any of the scripts to verify the block signature. Added a good description and an early verification failed return (before it was failing on the pubkey verification, when the problem is actually the staking transaction entirely).

Testing: 1) P2PK block signature pre v5 enforcement. 2) P2PK block signature post v5 enforcement. 3) P2PKH block signature pre v5 enforcement. 4) P2PKH block signature post v5 enforcement.

furszy · 2020-08-01T02:46:39Z

Updated per review.

random-zebra

utACK a7d4511 (after the rebase)

…rced conversion 50ee79d [wallet] Move loopTxsBalance and RecoverSaplingNote to use const reference inputs/iterators. (furszy) a46b837 [wallet] Remove now unneeded stake P2PKH->P2PK out forced conversion (furszy) 433a14e [wallet] Remove unreachable IsZPIV() in `CreateCoinStake` (furszy) Pull request description: Since v5 activation, #1700 is active and there is no need to continue checking for the v5 NU enforcement during the coinstake generation in the mining process. This PR removes the extra validations and script conversion inside the wallet. ACKs for top commit: random-zebra: re-utACK 50ee79d after rebase Fuzzbawls: utACK 50ee79d Tree-SHA512: ff1ce1de7ef14dee399e729a25ee4a80e791213e2470154e6cce9d79ed69b40ca557035d35be13d7c046b4e5f2c3d71a8bc13186c01c1c0479eceda204850e67

furszy self-assigned this Jun 20, 2020

furszy changed the title ~~Move coinbase & coinstake to P2PKH~~ [WIP] Move coinbase & coinstake to P2PKH Jun 20, 2020

furszy force-pushed the 2020_move_to_p2pkh branch from 7248a73 to 64805e0 Compare June 20, 2020 20:31

furszy changed the title ~~[WIP] Move coinbase & coinstake to P2PKH~~ Move coinbase & coinstake to P2PKH Jun 20, 2020

furszy force-pushed the 2020_move_to_p2pkh branch 2 times, most recently from 951de39 to f0daf85 Compare June 21, 2020 23:31

furszy added this to the 5.0.0 milestone Jun 27, 2020

furszy force-pushed the 2020_move_to_p2pkh branch from f0daf85 to cb6f9e3 Compare July 9, 2020 22:23

random-zebra suggested changes Jul 17, 2020

View reviewed changes

furszy force-pushed the 2020_move_to_p2pkh branch 4 times, most recently from eb86cc5 to 435af8f Compare July 20, 2020 16:13

random-zebra reviewed Jul 22, 2020

View reviewed changes

random-zebra added Block Generation Mining/Staking related issues Refactoring Validation labels Jul 22, 2020

Move coinbase output from p2pk to p2pkh

2cd8be4

furszy force-pushed the 2020_move_to_p2pkh branch from 435af8f to 3a27245 Compare July 22, 2020 18:11

random-zebra previously approved these changes Jul 22, 2020

View reviewed changes

furszy requested a review from Fuzzbawls July 31, 2020 00:41

Fuzzbawls previously approved these changes Aug 1, 2020

View reviewed changes

src/blocksignature.cpp Outdated Show resolved Hide resolved

src/stakeinput.cpp Outdated Show resolved Hide resolved

furszy added 7 commits July 31, 2020 23:44

Move coinstake output from p2pk to p2pkh.

1fad2ed

rpc_blockchain.py: decreased the amount of bytes

Consensus: P2PKH block signatures were always failing before v5 upgrade.

5bb6a31

Consensus: Do not create P2PKH outputs in the staking procedure pre v…

f385265

…5 upgrade. Only P2PK and P2CS outputs were accepted before block version 8 due a badly implemented block signature check.

Implemented a block signature verification unit test.

28ab884

Testing: 1) P2PK block signature pre v5 enforcement. 2) P2PK block signature post v5 enforcement. 3) P2PKH block signature pre v5 enforcement. 4) P2PKH block signature post v5 enforcement.

Simple test chain moving on v5 upgrade enforcement.

a7d4511

furszy dismissed stale reviews from Fuzzbawls and random-zebra via a7d4511 August 1, 2020 02:45

furszy force-pushed the 2020_move_to_p2pkh branch from 3a27245 to a7d4511 Compare August 1, 2020 02:45

random-zebra approved these changes Aug 1, 2020

View reviewed changes

furszy merged commit 4074208 into PIVX-Project:master Aug 1, 2020

Fuzzbawls mentioned this pull request Aug 2, 2020

Base work for the Sapling signatureHash #1666

Merged

random-zebra modified the milestones: 5.0.0, 4.3.0 Sep 10, 2020

furszy mentioned this pull request Apr 8, 2021

[wallet] Remove now unneeded stake P2PKH->P2PK output forced conversion #2299

Merged

furszy deleted the 2020_move_to_p2pkh branch November 29, 2022 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move coinbase & coinstake to P2PKH #1700

Move coinbase & coinstake to P2PKH #1700

furszy commented Jun 20, 2020

random-zebra commented Jun 22, 2020

furszy commented Jun 22, 2020

furszy commented Jul 15, 2020

random-zebra left a comment

furszy commented Jul 17, 2020

furszy commented Jul 17, 2020

furszy commented Jul 20, 2020

furszy commented Jul 20, 2020

random-zebra left a comment

furszy commented Jul 22, 2020

random-zebra left a comment

Fuzzbawls left a comment

furszy commented Aug 1, 2020

random-zebra left a comment

Move coinbase & coinstake to P2PKH #1700

Move coinbase & coinstake to P2PKH #1700

Conversation

furszy commented Jun 20, 2020

random-zebra commented Jun 22, 2020

furszy commented Jun 22, 2020

furszy commented Jul 15, 2020

random-zebra left a comment

Choose a reason for hiding this comment

furszy commented Jul 17, 2020

furszy commented Jul 17, 2020

furszy commented Jul 20, 2020

furszy commented Jul 20, 2020

random-zebra left a comment

Choose a reason for hiding this comment

furszy commented Jul 22, 2020

random-zebra left a comment

Choose a reason for hiding this comment

Fuzzbawls left a comment

Choose a reason for hiding this comment

furszy commented Aug 1, 2020

random-zebra left a comment

Choose a reason for hiding this comment