Implement and enforce DIP6 commitments #2477
Conversation
There was a problem hiding this comment.
I've only done an initial cursory review of this PR (edit: up to commit 7f27d231c). I love that we're able to get this in now.
I would also love if we could add documentation around new code introduced, esp. LLMQ namespace. Class level and methods would be great and allow us to set a higher standard for quality for our codebase moving forward. (We could get an intern to help w/this. 😉 )
src/llmq/quorums_blockprocessor.h
Outdated
There was a problem hiding this comment.
I know we have other places w/globals like this (e.g. sporkManager), but is there any way to avoid them in the new code?
There was a problem hiding this comment.
How? We need these object everywhere, so we need some type of global/singleton instance. The alternative is to put these into static class members, but IMHO this is in no way better then global variables. One big downsides of global variables is the cluttering of the global namespace, but this is only partially the case here as these live in their own namespace (llmq)
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
8021565 sounds like a bugfix which is not related to bip6, so it deserves its own PR imo.
Also, see a couple of notes below but otherwise looks pretty cool :)
codablock
force-pushed
the
pr_dip6_commitments
branch
from
5a612b0 to
b1a0b9a
Compare
But only for TRANSACTION_QUORUM_COMMITMENT for now.
This should avoid a fork on the current testnet. It only applies to the current chain which activated DIP3 at height 264000 and block 00000048e6e71d4bd90e7c456dcb94683ae832fcad13e1760d8283f7e89f332f. When we revert the chain to retest the DIP3 deployment, this fork logic can be removed again.
…lse statements Also add BEGIN/END markers for temporary code.
…entRequired This should make the code easier to read and also removes some duplication. The also changes the error types that are possible from 3 to 2 now. Instead of having "bad-qc-already-mined" and "bad-qc-not-mining-phase", there is only "bad-qc-not-allowed" now.
codablock
force-pushed
the
pr_dip6_commitments
branch
from
b1a0b9a to
662bee3
Compare
DIP6 quorum final commitment (dashpay/dash#2477)
* Content - RPC - Update quick reference * RPC - Update getblockchaininfo to show BIP-9 progress Related to dashpay/dash#2435 * RPC - Update gobject prepare with new params Use-IS (dashpay/dash#2452) Use specific UTXO for fee (dashpay/dash#2482) * RPC - Update mode name * RPC - Update protx default mode dashpay/dash#2513 * Content - Add spork 17 * Content - Special transactions Add info for Quorum commitment Remove messages not in 13.0 (SubTx) * P2P - Add new txlvote fields masternodeProTxHash (dashpay/dash#2484) quorumModifierHash (dashpay/dash#2505) * RPC - Update protx list Make all options follow the same parameter format (dashpay/dash#2559) * Content - version bump 0.13.0.0 bumped to 70213 (dashpay/dash#2557) * Guide - PrivateSend dstx message limit Up to 5 simultaneous dstxs per MN allowed (dashpay/dash#2552) * RPC - Update getblock Add missing versionHex field (dashpay/dash@e7d9ffa) Change to use verbosity syntax (dashpay/dash#2506 and bitcoin/bitcoin#8704) * P2P - Add qfcommit message (no hexdump example) DIP6 quorum final commitment (dashpay/dash#2477) * P2P - qfcommit typo Change description of llmqType field * P2P - Special tx payload size clarification * Guide - Update MN payment description Related to dashpay/dash#2258 * Guide - fix broken link * Guide - Update some example txs Change to hashes on the chain following the 12.3.4 reset * P2P - Add QcTx hexdump * P2P - DIP4 message updates Add SML entry Update hexdump to include new fields Add getmnlistd and mnlistdiff to cross ref * P2P - minor DIP3-related comments
3 participants
This is the first PR of a series of PRs resulting from an internal discussion we had. We need to have all on-chain consensus for LLMQs included into v13 already, as otherwise deployment of v14 will require miner support (BIP9 signaling) again.
The reason is that v13 will very likely ship without PoSe and we depend on v14 delivering rudimentary PoSe asap, so we should prepare ourselfs for a fast finalization and deployment of v14 with LLMQs.
The on-chain consensus changes include LLMQ commitments and the rudimentary PoSe system which is built on-top of the information gathered from successful LLMQ DKG sessions. The rudimentary PoSe system will be the next PR. Inclusion of these changes into v13 ensures a smooth upgrade to v14 later, as v13 (including miners) will be compatible.
As we'll have to test this properly on testnet, we will also have to include a "dummy DKG" implementation. This dummy DKG would be insecure by definition and ONLY FOR TESTNET. The reason for using the dummy DKG first is that it will be easier/faster to review.
If review of the real (non-dummy) DKG goes well and is fast enough, we might also include the real DKG and start testing it on testnet (not active on mainnet).
This PR also includes some simple fork-logic so that we can activate enforcement and validation of commitments and null commitments on testnet. The plan for mainnet is to activate this together with the BIP9 deployment, but as the BIP9 deployment has already activated at block 264000 on testnet, we'll have to handle this properly as otherwise the chain would have to be reverted (loosing all registered DIP3 MNs). If review of this PR takes longer, I'll bump the activation height for testnet by a few hundred/thousand blocks.
This PR introduces 3 different LLMQ types for mainnet/testnet/devnet:
This one is targeted for InstantSend
This one is targeted for consensus critical decisions that require more security
This one is targeted for signalling of minimum protocol versions and deployment logic.
I've prepared my llmq branch to be based on this PR, in case you want to see what comes next. The commits planned for v13 are up until "Test simple PoSe in DIP3 tests". Everything that comes after that commit is for v14.