Merge master / 23.1.4 into develop#7367
Conversation
…ssing changes from bitcoin#19607 eb769ab fix: remove cs_main from PeerManagerImpl::MaybePunishNodeForTx, missing changes from bitcoin#19607 (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Helper MaybePunishNodeForTx has been introduced when bitcoin#19607 is done, it caused missing changes. ## What was done? Removed cs_main from MaybePunishNodeForTx ## How Has This Been Tested? N/A ## Breaking Changes N/A ## Checklist: - [x] 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: PastaPastaPasta: utACK eb769ab Tree-SHA512: ce9b26d5dd3036d1b8b5ff7f302e813b6c3e776f0132d8cdca5ccabbd9f485fd93cd55629c73d576362723f6f35fcc528c4be0ad3d3ae64aca6dd4546d913cbd
…et-pow calculation much faster e8435d3 test: add regression test for faster division of base_uint<256> to uint32_t (Konstantin Akimov) de1e56a perf: optimize division to uint32 to make re-target-pow calculation much faster (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Dash retargets difficulty every block via DarkGravityWave (24-block loop), so every header processed during sync runs several 256-bit divisions. Bitcoin only retargets once per 2016 blocks. Every division in DGW is by a small integer (nCountBlocks+1) * nTargetTimespan (3600). But the code routes through the only available overload, `operator/=(const base_uint&)`, which does bit-by-bit long division: ~224 iterations, each doing a full-width `div >>= 1` During header sync up to 35% of CPU is wasted on this division on release build (accordingly `perf`): ``` 20.36% d-msghand dash-qt [.] base_uint<256u>::operator>>=(unsigned int) 15.00% d-msghand dash-qt [.] base_uint<256u>::operator/=(base_uint<256u> const&) ``` ## What was done? Implemented optimization for division to unsigned values that could fit inside uint32_t. ## How Has This Been Tested? Got GUIX build from changes in this PR and develop: ``` [develop] 5af9f57/src/qt/dash-qt -datadir=/tmp/dd -reindex real 7m42.152s user 7m0.396s sys 0m3.328s [PR] 8608a3c3bbcd/src/qt/dash-qt -datadir=/tmp/dd -reindex real 4m17.294s user 2m55.474s sys 0m2.731s ``` Perf stats are updated as expected: ``` - 1.61% 0.06% d-msghand dash-qt [.] base_uint<256u>::operator/=(base_uint<256u> const&) - 1.55% base_uint<256u>::operator/=(base_uint<256u> const&) 0.92% base_uint<256u>::operator>>=(unsigned int) + 1.48% 0.57% d-msghand dash-qt [.] base_uint<256u>::operator>>=(unsigned int) ``` So total header sync just a half of the time. Please note, that header sync will be slightly slower due to dashpay#7320 so overall win is smaller. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] 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 e8435d3 Tree-SHA512: 797c446cbfad6a1eecb98bee893d530e88db7576d0d15faa36c0d9ccb6d3872313c29f977194ba42cfdedf837a7227018dc750fa87b1f376106831204b08f650
It also prevent potential crash in assert in case of huge re-organizations of blockchain when buried fork is re-validated in BlockUndo
c8801b2 test: add test for non-standard asset-locks (Konstantin Akimov) 9ad2afb feat: make v2 asset locks and huge asset locks as non-standard (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Platform ignores asset-locks with amount of inputs more than 100 or with size of transaction bigger than 20480 bytes. If user will create too big asset-lock transaction that platform (L2) can not recognize it may cause to fund lost. Issue is worsened once user will have ability to create asset-lock tx himself by Dash Core after dashpay#7294 It's an addition to this PR: https://github.com/dashpay/platform/pull/3491/files ## What was done? Asset locks that have more than 100 inputs are non-standard txes Asset locks that have size bigger than 20480 bytes are non-standard txes Asset locks that have v2 payload are non-standard txes. Once v24 is activated, platform may be not ready to process them. This non-standard limitation is a feature for soft enabling v2 asset-locks without 2nd fork. ## How Has This Been Tested? See updates in functional test. ## Breaking Changes Some special transactions are now valid for consensus if they are mined in block but they are non-standard and won't be relayed to network. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone Top commit has no ACKs. Tree-SHA512: ce2a6221f66367d839696ce2bfd93941a74eae1469276703fae06e1ccb3f979beaf14ca4dc05c75d03496cdabb5ae58d6b87b902c0a7bfb16e42d558cbb10d96
|
✅ Review complete (commit 239d8da) |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 239d8da65e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| {2361500, uint256S("0x0000000000000009ba1e8f47851d036bb618a4f6565eb3c32d1f647d450ff195")}, | ||
| {2421800, uint256S("0x000000000000000718ed026ebd644a8b70b42d4cbd7b25304c066c9bf15f85b7")}, | ||
| {2471728, uint256S("0x000000000000001a19ad7270422a00f86123ea94e0b295a3a796d6861bd7b032")}, | ||
| {2487500, uint256S("0x00000000000000000000000000000000000000000000ba7225adc3153945d14d")}, |
There was a problem hiding this comment.
This checkpoint value is the chain-work literal just assigned to consensus.nMinimumChainWork, not the block hash for height 2487500; the block hash for that height is already listed in defaultAssumeValid above. Checkpoint lookup treats these values as block hashes via LookupBlockIndex(hash), so this newest checkpoint will never be found and checkpoint protection silently remains at the previous height instead of the advertised 2487500.
Useful? React with 👍 / 👎.
WalkthroughThis PR prepares the Dash Core v23.1.4 release. The build version is incremented from 3 to 4 in Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (2)
src/evo/mnhftx.cpp (1)
181-181: 💤 Low valueConsider dereferencing
bitfor consistency with line 184.At this point
bit.has_value()is guaranteed true. Using*bitinstead ofbitwould be more explicit and consistent with thepush_back(*bit)on line 184.- if (std::find(new_signals.begin(), new_signals.end(), bit) != new_signals.end()) { + if (std::find(new_signals.begin(), new_signals.end(), *bit) != new_signals.end()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/evo/mnhftx.cpp` at line 181, In the std::find call on line 181, replace the `bit` parameter with `*bit` to dereference the optional value. Since `bit.has_value()` is guaranteed to be true at this point in the code, using the dereferenced value `*bit` makes the code more explicit and consistent with the `push_back(*bit)` call on line 184. This ensures uniform handling of the bit variable throughout this code block.doc/release-notes/dash/release-notes-23.1.3.md (1)
3-3: ⚡ Quick winUse "bug fixes" (two words) instead of "bugfixes" for consistency.
Line 3 uses "bugfixes" as a single word, but the document elsewhere (line 28 "## Bug Fixes") and Dash documentation standards use "bug fixes" as two words. Align terminology for consistency.
🔧 Proposed fix
-This is a new patch version release, bringing GUI improvements and bugfixes. +This is a new patch version release, bringing GUI improvements and bug fixes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doc/release-notes/dash/release-notes-23.1.3.md` at line 3, In the release notes introduction sentence, change the single-word "bugfixes" to the two-word phrase "bug fixes" to maintain consistency with the document's section headers and Dash documentation standards. This terminology alignment ensures the document follows a consistent style throughout, matching the "## Bug Fixes" section heading and established conventions.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chainparams.cpp`:
- Around line 221-222: There is a copy/paste error in the checkpointData
structure around line 339 where a chainwork value has been mistakenly used
instead of a block hash. Locate the checkpointData structure entry at line 339
and replace the incorrect hash value (which currently matches the chainwork
value from the consensus.nMinimumChainWork assignment in lines 221-222) with the
correct block hash that should be used as the checkpoint entry for that block
height. Ensure that checkpoint entries contain actual block hashes rather than
chainwork calculations.
---
Nitpick comments:
In `@doc/release-notes/dash/release-notes-23.1.3.md`:
- Line 3: In the release notes introduction sentence, change the single-word
"bugfixes" to the two-word phrase "bug fixes" to maintain consistency with the
document's section headers and Dash documentation standards. This terminology
alignment ensures the document follows a consistent style throughout, matching
the "## Bug Fixes" section heading and established conventions.
In `@src/evo/mnhftx.cpp`:
- Line 181: In the std::find call on line 181, replace the `bit` parameter with
`*bit` to dereference the optional value. Since `bit.has_value()` is guaranteed
to be true at this point in the code, using the dereferenced value `*bit` makes
the code more explicit and consistent with the `push_back(*bit)` call on line
184. This ensures uniform handling of the bit variable throughout this code
block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 58a9fcb6-fda6-4273-a480-1cd12d255e8b
📒 Files selected for processing (19)
configure.accontrib/flatpak/org.dash.dash-core.metainfo.xmlcontrib/seeds/nodes_main.txtcontrib/seeds/nodes_test.txtdoc/man/dash-cli.1doc/man/dash-qt.1doc/man/dash-util.1doc/man/dash-wallet.1doc/man/dashd.1doc/release-notes.mddoc/release-notes/dash/release-notes-23.1.3.mdsrc/chainparams.cppsrc/chainparamsseeds.hsrc/evo/chainhelper.cppsrc/evo/mnhftx.cppsrc/evo/mnhftx.hsrc/txmempool.cppsrc/txmempool.htest/functional/rpc_netinfo.py
| consensus.nMinimumChainWork = uint256S("0x00000000000000000000000000000000000000000000ba7225adc3153945d14d"); // 2487500 | ||
| consensus.defaultAssumeValid = uint256S("0x00000000000000119fe42827219e0686d3f7b494ae65f823194c740c5dbab492"); // 2487500 |
There was a problem hiding this comment.
Checkpoint entry at Line 339 appears to use chainwork instead of a block hash.
Line 339 duplicates the chainwork value from Line 221, which strongly suggests a copy/paste error in checkpointData. Checkpoint entries should store block hashes.
Proposed fix
- {2487500, uint256S("0x00000000000000000000000000000000000000000000ba7225adc3153945d14d")},
+ {2487500, uint256S("0x00000000000000119fe42827219e0686d3f7b494ae65f823194c740c5dbab492")},Also applies to: 339-339
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chainparams.cpp` around lines 221 - 222, There is a copy/paste error in
the checkpointData structure around line 339 where a chainwork value has been
mistakenly used instead of a block hash. Locate the checkpointData structure
entry at line 339 and replace the incorrect hash value (which currently matches
the chainwork value from the consensus.nMinimumChainWork assignment in lines
221-222) with the correct block hash that should be used as the checkpoint entry
for that block height. Ensure that checkpoint entries contain actual block
hashes rather than chainwork calculations.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified PR #7367 at 239d8da. Found one in-scope release-notes consistency issue: the merge updates the file body, changelog heading, older-release list, and compare link for v23.1.4, but the top heading/opening sentence still say v23.1.3. Review coverage was degraded: codex-general produced the parseable finding; claude-general failed due an ACP runtime/quota limit; the verifier was rerun with codex because the Claude verifier hit the same limit; no CodeRabbit findings were provided.
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `doc/release-notes.md`:
- [SUGGESTION] doc/release-notes.md:1-3: Fix stale v23.1.3 release-note header
The PR updates this file's release contents, changelog heading, older-release list, and compare link for v23.1.4, but the document still opens as v23.1.3 and describes the previous GUI-focused patch release. That leaves the current release notes internally inconsistent for the v23.1.4 merge into develop.
3 participants
Issue being fixed or feature implemented
merge of
master(post-v23.1.4release) back intodevelop. Brings the 8 commits released as part ofv23.1.4into the active development branch.What was done?
Merged
upstream/master(tip6f7a0eff4d7,chore: version bump […] for release v23.1.4) intodevelop. Three files conflicted and were resolved manually; everything else auto-merged.Conflict resolutions:
configure.ac— Took master's_CLIENT_VERSION_BUILD=4but kept develop's_CLIENT_VERSION_IS_RELEASE=false(develop is not a release branch).src/evo/chainhelper.cpp— Two unrelated changes collided:CMNPaymentsProcessorto usegovernance::SuperblockManagerinstead ofCGovernanceManager(no longer needssporkman/mn_syncas constructor args).Merge #7332(fix: remove cs_main from MaybePunishNodeForTx) is already present in develop history (commit9dd249c8720), applied to the pre-refactor form ofCMNPaymentsProcessor. The constructor-signature change from that PR was therefore suppressed — develop's new signature is the source of truth.perf: avoid re-validation of ehf signals during block-connect(54187cd0bb4): adopted the drop ofqmanfromCMNHFManager(...). Kept develop'ssuperblocksmember andmn_paymentsconstruction unchanged.src/evo/mnhftx.cpp— Took master's perf rewrite ofextractSignals(no longer re-validates each MNHF tx during block connect; only collects version bits). Develop's older inline re-validation was suppressed. Also removed the now-unused<util/std23.h>include sincestd23::ranges::containsis no longer called here.How Has This Been Tested?
make -j8) —dashd,dash-cli,dash-tx,dash-wallet,dash-util,dash-chainstate,test/test_dash,bench/bench_dash,test/fuzz/fuzzall linked clean.Breaking Changes
None. All consensus-relevant changes from
masterwere already merged intodevelopvia prior backport PRs; this merge only reconciles the version metadata and brings forward the EHF signal block-connect perf optimization.Checklist: