Sigma pool closed, Extra payload extended #1477
Conversation
WalkthroughThe changes involve significant modifications to the consensus parameters of a blockchain application, including the removal of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Blockchain
participant Miner
participant Validator
User->>Blockchain: Initiate Transaction
Blockchain->>Validator: Validate Transaction
Validator-->>Blockchain: Transaction Valid
Blockchain->>Miner: Create Block
Miner->>Blockchain: Add Block
Blockchain->>User: Confirm Transaction
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- src/chainparams.cpp (6 hunks)
- src/consensus/consensus.h (1 hunks)
- src/consensus/params.h (2 hunks)
- src/lelantus.cpp (1 hunks)
- src/miner.cpp (2 hunks)
- src/test/firsthalving_tests.cpp (2 hunks)
- src/test/main_tests.cpp (2 hunks)
- src/validation.cpp (6 hunks)
- src/wallet/lelantusjoinsplitbuilder.cpp (1 hunks)
Additional comments not posted (27)
src/test/main_tests.cpp (4)
24-24: LGTM!The code changes are approved.
26-26: LGTM!The code changes are approved.
27-28: LGTM!The code changes are approved.
29-30: LGTM!The code changes are approved.
src/consensus/consensus.h (1)
39-39: Approve the addition ofNEW_MAX_TX_EXTRA_PAYLOADconstant.The addition of the new constant is approved as it does not modify any existing code.
Verify the usage and impact of
NEW_MAX_TX_EXTRA_PAYLOADin the codebase.Please ensure that:
- The constant is used correctly in all relevant parts of the codebase.
- The increased payload size does not have any unintended consequences on transaction processing, storage, or network performance.
- Proper validation is in place to enforce the new payload size limit.
Run the following script to verify the constant usage:
Verification successful
Usage of
NEW_MAX_TX_EXTRA_PAYLOADconfirmed in transaction validation.The
NEW_MAX_TX_EXTRA_PAYLOADconstant is used insrc/validation.cppto enforce payload size limits in transactions. This confirms its role in the validation logic, ensuring that the new limit is applied after a specific block height.
- Location of usage:
src/validation.cpp: The constant is used to validate transaction payload sizes.Please ensure that this logic is thoroughly tested to prevent any unintended consequences on transaction processing. If further verification is needed, consider reviewing test coverage for this validation path.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage and impact of `NEW_MAX_TX_EXTRA_PAYLOAD` in the codebase. # Test 1: Search for the constant usage. Expect: At least one occurrence in transaction handling code. rg --type c++ -A 5 $'NEW_MAX_TX_EXTRA_PAYLOAD' # Test 2: Search for payload size validation. Expect: Validation against the new limit. rg --type c++ -A 5 $'(tx|transaction).*extra.*payload.*size'Length of output: 163
Script:
#!/bin/bash # Description: Verify the usage and impact of `NEW_MAX_TX_EXTRA_PAYLOAD` in the codebase. # Test 1: Search for the constant usage. Expect: At least one occurrence in transaction handling code. rg -A 5 'NEW_MAX_TX_EXTRA_PAYLOAD' # Test 2: Search for payload size validation. Expect: Validation against the new limit. rg -A 5 '(tx|transaction).*extra.*payload.*size'Length of output: 986
src/test/firsthalving_tests.cpp (3)
Line range hint
115-267: LGTM!The test case provides thorough coverage for verifying the subsidy halving mechanism and developer payouts. The test case logic looks good.
Line range hint
269-309: LGTM!The test case provides additional coverage for verifying the consensus rules related to developer payouts. The test case logic looks good.
Line range hint
115-115: Verify the impact of removingnSubsidyHalvingStopBlockon the halving mechanism.Ensure that the removal of this parameter does not introduce any unintended consequences or bugs in the subsidy halving logic.
Run the following script to verify the halving mechanism:
Verification successful
Subsidy Halving Mechanism Verified
The removal of
nSubsidyHalvingStopBlockdoes not negatively impact the subsidy halving mechanism. The logic is now governed bynSubsidyHalvingFirst,nSubsidyHalvingSecond, andnSubsidyHalvingInterval, and is thoroughly tested in the codebase.
- Functions like
GetBlockSubsidyWithMTPFlagandGetBlockSubsidyimplement the subsidy calculation logic.- Test cases in
firsthalving_tests.cppand other files verify the correct application of the halving logic.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing `nSubsidyHalvingStopBlock` on the halving mechanism. # Test: Search for potential issues related to the removal of `nSubsidyHalvingStopBlock`. # Expect: No concerning usages or potential bugs. rg --type cpp -i 'nSubsidyHalvingStopBlock' -A 5Length of output: 48
Script:
#!/bin/bash # Description: Search for functions or logic related to subsidy calculations and halving intervals. # Search for subsidy calculation functions or logic in the codebase. rg --type cpp -i 'subsidy' -A 10 # Search for halving interval logic in the codebase. rg --type cpp -i 'halving' -A 10Length of output: 80171
src/consensus/params.h (2)
164-172: New consensus parameters for stage 4 coinbase payment distribution.The new parameters
stage4StartBlock,stage4DevelopmentFundShare,stage4CommunityFundShare, andstage4MasternodeSharecollectively define the coinbase payment distribution for stage 4.Please verify the following:
- The
stage4StartBlockis set to the intended block height for the start of stage 4.- The fund share percentages (
stage4DevelopmentFundShare,stage4CommunityFundShare,stage4MasternodeShare) add up to 100%.- The corresponding fund addresses for stage 4 are correctly set in the codebase.
174-175: Introduction of tail emission after stage 4.The new parameter
tailEmissionBlockSubsidyindicates that there will be a perpetual block subsidy even after stage 4, creating a tail emission of coins.Introducing a tail emission is a significant change to the coin emission schedule. Please verify the following:
- The
tailEmissionBlockSubsidyamount is carefully chosen, taking into account the long-term supply and economic implications of the tail emission.- The block reward calculation logic properly handles the activation of the tail emission based on the current block height.
src/wallet/lelantusjoinsplitbuilder.cpp (1)
196-198: LGTM!The added guard clause enhances the robustness of the
Buildmethod by preventing further operations related to the Sigma pool if it has already been closed at the specified block height (stage4StartBlock). This ensures that the method does not proceed under invalid conditions.src/miner.cpp (1)
838-863: Significant changes to introduce stage 4 funding period. LGTM!The key changes are:
- Introduction of stage 4 funding period starting at block height
params.stage4StartBlock.- Removal of
nSubsidyHalvingStopBlockparameter.- New parameters
stage4DevelopmentFundShareandstage4CommunityFundShareto define the payout percentages for stage 4.- Updated logic to calculate the development and community fund payouts based on the current stage (3 or 4).
Please verify the following:
- The payout addresses
params.stage3DevelopmentFundAddressandparams.stage3CommunityFundAddressare correct for stage 3.- The payout percentages
params.stage3DevelopmentFundShare,params.stage3CommunityFundShare,params.stage4DevelopmentFundShare, andparams.stage4CommunityFundShareare as per the consensus.- The payouts are correctly deducted from the block subsidy calculated using
GetBlockSubsidyWithMTPFlag().src/chainparams.cpp (7)
208-213: Changes look good!The changes introduce the new stage 4 funding parameters and remove the
nSubsidyHalvingStopBlockparameter as described in the summary. The values assigned to the new parameters also look correct.
535-540: LGTM!The stage 4 funding parameter changes for the test network look good and are consistent with the changes made to the main network parameters.
802-803: Please confirm the intention behind the aggressive halving schedule changes for the dev network.The
nSubsidyHalvingSecondandnSubsidyHalvingIntervalvalues have been significantly reduced, which will cause the block subsidy to halve much faster than before on the dev network.Please confirm this is intentional for testing/experimental purposes and won't have any unintended side-effects.
817-822: Stage 4 funding parameter additions look good.The new stage 4 funding parameters have been added consistently to the dev network parameters, similar to the main and test networks. The values assigned also look suitable for the dev network.
1056-1056: The stage 3 start block change for regtest looks fine.Delaying the start of stage 3 to block 2000 on regtest is a reasonable change for testing purposes and shouldn't cause any issues.
1063-1068: The stage 4 parameter values for regtest look acceptable.Although the values differ from the other networks, with a higher development fund share and lower masternode share, these should be okay for regression testing purposes. The tail emission block subsidy matches the other networks.
Line range hint
1275-1275: The updated regtest genesis block parameters look good!The changes to the genesis block parameters for regtest:
- nTime set to
ZC_GENESIS_BLOCK_TIME- nNonce updated to a new value
- nBits set to the PoW limit of 0x207fffff
- nVersion changed to 1
- genesisReward set to 0
appear intentional and reasonable for testing purposes. Using the PoW limit for nBits and 0 block reward are common practices for regtest genesis blocks.
src/lelantus.cpp (1)
424-429: LGTM!The new conditional check correctly prevents processing Sigma-to-Lelantus joinsplit transactions after the
stage4StartBlockheight, effectively closing the Sigma pool as intended.src/validation.cpp (7)
Line range hint
665-670: Allows larger transaction payload sizes after a protocol upgrade height. LGTM.The new check allows
MAX_TX_EXTRA_PAYLOADbytes of extra payload data before::Params().GetConsensus().stage4StartBlockheight, andNEW_MAX_TX_EXTRA_PAYLOADbytes after that.This provides a clean upgrade path for increasing the supported extra payload size at a later height. The check itself looks good.
1926-1938: Introduces a new block subsidy schedule with a tail emission. Looks good!The updated
GetBlockSubsidyWithMTPFlag()function defines a new block subsidy schedule:
- The original BTC-like halving schedule is used up to
nSubsidyHalvingSecondheight- A 3rd "stage" cuts the subsidy in half once more
- Finally, a 4th "stage" introduces a fixed "tail emission" subsidy of
consensusParams.tailEmissionBlockSubsidyThis provides a smooth transition to a supply curve with no hard cap, while still preserving the original halving behavior for a long time.
The staging logic using block height ranges looks good. The tail emission avoids some economic issues with a hard cap.
1957-1961: Simplifies masternode payments after stage 4 start to use a fixed percentage. LGTM.The
GetMasternodePayment()function is updated to use a fixed percentage of the block reward for masternode payments, starting at thestage4StartBlockheight.This removes the time-based conditions in the old logic, making the payments simpler and more predictable. The change looks good:
blockValue*params.stage4MasternodeShare/100is used after the stage 4 start height- The old logic is kept for earlier blocks for backwards compatibility
This is a nice simplification of the masternode payment logic.
Line range hint
4840-4864: Implements new consensus rules to enforce development and community fund payouts. Looks good!The new contextual checks verify the coinbase transaction contains outputs paying the expected amounts to the development and community funds.
Two payout schemes are enforced:
- After
stage3StartTime:
- Both dev and community fund payouts are required
- The payout amounts are calculated using the block subsidy and stage-specific share percentages (
stage3DevelopmentFundShare,stage4DevelopmentFundShare, etc)- The stage 3 percentages are used up to
stage4StartBlockheight, after which stage 4 percentages apply- Between
nSubsidyHalvingFirstandstage3StartTime:
- Only the dev fund payout is required, using the
stage2DevelopmentFundSharepercentageThe payout amounts look to be calculated correctly for each stage, and the logic to scan the coinbase outputs for the expected payments is implemented properly.
Overall this is a good addition to enforce the dev/community fund payouts!
4866-4875: Enforces "stage 2" development fund payouts between the first halving and stage 3 start. Looks good.This code block implements the payout checks for the "stage 2" development fund, which is in effect between the
nSubsidyHalvingFirstandstage3StartTimeheights.During stage 2, a portion of the block subsidy on each block is expected to be paid to the dev fund. The expected payout amount is calculated as:
devPayoutValue = (GetBlockSubsidy() * consensusParams.stage2DevelopmentFundShare) / 100The code then verifies the coinbase transaction contains an output paying
devPayoutValueto thestage2DevelopmentFundAddress. If the output is not found, an error is returned.The payout calculation and output check look correct for stage 2. This properly enforces the expected dev fund payments during this period.
4847-4852: Calculates the expected development and community fund payouts for stage 3 and later. Looks good.This code calculates the expected dev and community fund payout amounts for blocks after the
stage3StartTime.The payout amounts are calculated differently depending on whether the block is in stage 3 or stage 4 and later:
- For stage 3 (block height <
stage4StartBlock), thestage3DevelopmentFundShareandstage3CommunityFundSharepercentages are used- For stage 4+ (block height >=
stage4StartBlock), thestage4DevelopmentFundShareandstage4CommunityFundSharepercentages are usedThe code selects the appropriate percentages based on the block height, then calculates the payout amounts as:
devPayoutValue = (GetBlockSubsidy() * devShare) / 100 communityPayoutValue = (GetBlockSubsidy() * communityShare) / 100The subsidy and percentage values are all pulled from the consensus parameters.
This logic looks correct for calculating the expected payouts based on the block's stage. Using different share percentages for each stage allows for flexibility in the payout scheme.
Line range hint
4853-4863: Verifies the coinbase transaction contains the expected dev and community fund payouts. Looks good.This code scans the outputs of the coinbase transaction to verify the block pays the expected amounts to the development and community funds.
For each coinbase output, it checks if the output script matches either:
- The dev fund script (
devPayoutScript) and the output amount equals the expected dev payout (devPayoutValue), or- The community fund script (
communityPayoutScript) and the amount equals the expected community payout (communityPayoutValue)Boolean flags
devFoundandcommunityFoundare used to track whether a valid output was found for each fund.After scanning all outputs, the code checks that both
devFoundandcommunityFoundare true. If either flag is still false, it means the corresponding payout was missing from the coinbase outputs.In that case, an error is returned with a clear message indicating the block is invalid due to a missing dev or community payout.
This logic looks correct for enforcing the presence of the
reubenyap
changed the title
Tested on regtest !
We need to put new HF block number before merge.