-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: bump assume valid / checkpoints for v22.1 #6553
chore: bump assume valid / checkpoints for v22.1 #6553
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK aa6593a
WalkthroughThe pull request updates consensus parameters in the blockchain configuration file. Specifically, it changes the values for the minimum accumulated proof-of-work ( Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/chainparams.cpp
(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Dependencies (linux64, x86_64-pc-linux-gnu)
- GitHub Check: Build Dependencies (arm-linux, arm-linux-gnueabihf)
🔇 Additional comments (3)
src/chainparams.cpp (3)
334-335
: LGTM! Checkpoint block hash matches defaultAssumeValid.The new checkpoint at block height 2216986 is correctly added with a hash that matches the defaultAssumeValid parameter.
508-509
: LGTM! Testnet checkpoint block hash matches defaultAssumeValid.The new checkpoint at block height 1189000 is correctly added with a hash that matches the defaultAssumeValid parameter.
419-423
: Verify the consensus parameter updates for the test network.The testnet consensus parameters have been updated to use block 1189000 as reference. This is a higher block height than the previous values, which is expected for a version update.
Run the following script to verify the block exists and has the correct hash:
// The best chain should have at least this much work. | ||
consensus.nMinimumChainWork = uint256S("0x000000000000000000000000000000000000000000009eb0f1d7fefc8750aebb"); // 2175051 | ||
consensus.nMinimumChainWork = uint256S("0x00000000000000000000000000000000000000000000a39050764808db046f5c"); // 2216986 | ||
|
||
// By default assume that the signatures in ancestors of this block are valid. | ||
consensus.defaultAssumeValid = uint256S("0x000000000000001cf26547602d982dcaa909231bbcd1e70c0eb3c65de25473ba"); // 2175051 | ||
consensus.defaultAssumeValid = uint256S("0x0000000000000010b1135dc743f27f6fc8a138c6420a9d963fc676f96c2048f4"); // 2216986 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Consensus Parameter Verification Issue – Block Not Found
The block hash referenced in defaultAssumeValid
(0x0000000000000010b1135dc743f27f6fc8a138c6420a9d963fc676f96c2048f4) was not found using the current block explorer API. This suggests that either:
- The block corresponding to the updated consensus parameters (at height 2216986) has not been incorporated into the block explorer’s database, or
- The updated block hash is incorrect and needs re-verification.
Please double-check the hash value against internal chain data or alternative block explorers to ensure the parameters are consistent with the intended network upgrade.
🔗 Analysis chain
Verify the consensus parameter updates for the main network.
The consensus parameters have been updated to use block 2216986 as reference. This is a higher block height than the previous values, which is expected for a version update.
Run the following script to verify the block exists and has the correct hash:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the block exists and has the correct hash
# Note: This assumes the block explorer API is available
curl -s "https://insight.dash.org/insight-api/block/0x0000000000000010b1135dc743f27f6fc8a138c6420a9d963fc676f96c2048f4"
Length of output: 129
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try removing 0x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@UdjinM6 Perfect! I can now confirm that the block exists and the consensus parameters are correct. The block explorer shows block 2216986 with the exact hash and chainwork values as specified in the updated code.
(^▽^)
🔗 Analysis chain
@UdjinM6 Thank you for catching that! You're right - removing the 0x
prefix from the block hash allows the block explorer API to properly recognize the hash. I'll make sure to verify block hashes without the 0x
prefix in future reviews.
(`^´)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the block exists and has the correct hash
# Note: This assumes the block explorer API is available
curl -s "https://insight.dash.org/insight-api/block/0000000000000010b1135dc743f27f6fc8a138c6420a9d963fc676f96c2048f4"
Length of output: 1995
✏️ Learnings added
Learnt from: UdjinM6
PR: dashpay/dash#6553
File: src/chainparams.cpp:221-225
Timestamp: 2025-02-04T14:35:54.113Z
Learning: Block hashes should be verified without the '0x' prefix when using block explorer APIs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK aa6593a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK - matches my node 👍
aa6593a chore: bump assume valid / checkpoints for v22.1 (pasta) Pull request description: ## Issue being fixed or feature implemented Bump assume valid / checkpoint for upcoming 22.1 release ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg: utACK aa6593a UdjinM6: utACK aa6593a Tree-SHA512: 99755f527b94a1be4ead3dd3a5171c6f00c77b1807e005e64a813cce9e5299c507cf47115cfd07a11f86f15f6c9e1247c3d3635bb1c9e69d4930a363499f98f8
e62b4eb Merge #6555: backport: Merge bitcoin#24263: doc: Fix gen-manpages, rewrite in Python (pasta) 53d0ff1 Merge #6552: docs: update supported versions in SECURITY.md (pasta) 4571d89 Merge #6554: docs: update man pages for 22.1 (pasta) bc91a83 Merge #6553: chore: bump assume valid / checkpoints for v22.1 (pasta) Pull request description: ## Issue being fixed or feature implemented Backports from develop to 22.1 ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK e62b4eb Tree-SHA512: 9f88c1a77ec1adc453b023b764f91760e13064c86d109c29bb7856f2588c6450e518c9479f8c3e245f7eeaf5ef6d28aa85f00b4b5e94ef5a4d80cb5b3d7fdff2
774a018 chore: set release to true (pasta) e62b4eb Merge #6555: backport: Merge bitcoin#24263: doc: Fix gen-manpages, rewrite in Python (pasta) 53d0ff1 Merge #6552: docs: update supported versions in SECURITY.md (pasta) 4571d89 Merge #6554: docs: update man pages for 22.1 (pasta) bc91a83 Merge #6553: chore: bump assume valid / checkpoints for v22.1 (pasta) c11ec40 docs: add release notes for 22.1.0 (pasta) Pull request description: ## Issue being fixed or feature implemented Manually suppressed configure.ac changes ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK fd51512 kwvg: utACK fd51512 Tree-SHA512: 41f08e1879596c0dd339209f869f2e4c7497f275df1928154ed8dfd2df1ff66c0618792afbff8313a818ac4d14ed8aa04f00e0fa96468adcc494042a7a28cc2b
Issue being fixed or feature implemented
Bump assume valid / checkpoint for upcoming 22.1 release
What was done?
How Has This Been Tested?
Breaking Changes
Checklist: