fix: compute SML entry hash with SER_GETHASH semantics in dash#798
Open
xdustinface wants to merge 1 commit into
Open
fix: compute SML entry hash with SER_GETHASH semantics in dash#798xdustinface wants to merge 1 commit into
SER_GETHASH semantics in dash#798xdustinface wants to merge 1 commit into
Conversation
`MasternodeListEntry::calculate_entry_hash` hashed the full network consensus serialization, including the leading `version`. Dash Core's `CSimplifiedMNListEntry::CalcHash` uses `CHashWriter(SER_GETHASH, ...)`, and `SER_GETHASH` does not set `SER_NETWORK`, so the `SER_NETWORK`-gated leading `nVersion` is omitted from the hash pre-image. The two contexts therefore diverged for every entry. Extract the post-`version` body of the wire encoder into a shared `consensus_encode_body` helper. `consensus_encode` writes `version` then the body (byte-identical to before), while `calculate_entry_hash` hashes the body alone, matching Core's `CalcHash`. Verified against Core for both a `version` 1 and a `version` 2 Evo entry. Also fix `SocketAddr` decoding to use `to_ipv4_mapped` instead of `to_ipv4`, so an all-zero (`::`) service address round-trips to the same 16 bytes instead of acquiring an `::ffff:` mapped prefix. The prefix corruption changed the wire bytes and thus the entry hash for masternodes with an unset service.
Contributor
📝 WalkthroughWalkthroughThe PR refactors MasternodeListEntry consensus encoding to separate version-dependent logic from body serialization, updates hash calculation to use the new body-only preimage, and corrects IPv6 address decoding to properly handle IPv4-mapped addresses. ChangesConsensus Encoding and Hashing Alignment
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dash/src/sml/masternode_list_entry/hash.rs (1)
6-10: 💤 Low valueConsider returning
Resultor documenting infallibility.The
expect("encoding failed")violates the guideline to avoidexpect()in library code. However, encoding to aVec<u8>is practically infallible sinceVec'sWriteimpl only fails on allocation exhaustion (which would panic anyway).Two options:
- Keep as-is but add a comment explaining why failure is impossible
- Change signature to return
Result<sha256d::Hash, std::io::Error>for API consistencyGiven that this is a public method and allocation failure would abort regardless, option 1 is reasonable here.
📝 Suggested documentation
impl MasternodeListEntry { pub fn calculate_entry_hash(&self) -> sha256d::Hash { + // Vec<u8> write is infallible (barring OOM which aborts), so expect is safe here. let mut writer = Vec::new(); self.consensus_encode_body(&mut writer).expect("encoding failed"); sha256d::Hash::hash(&writer) } }As per coding guidelines: "Avoid
unwrap()andexpect()in library code; use proper error types".🤖 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 `@dash/src/sml/masternode_list_entry/hash.rs` around lines 6 - 10, The method calculate_entry_hash currently calls self.consensus_encode_body(&mut writer).expect("encoding failed"), which uses expect in library code; replace this by keeping the current behavior but add a concise comment explaining why encoding to a Vec<u8> is effectively infallible (the Write impl for Vec only fails on allocation exhaustion which would abort) and that returning a Result was considered but omitted for API stability; reference the calculate_entry_hash and consensus_encode_body symbols and state that sha256d::Hash::hash(&writer) remains unchanged.
🤖 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.
Nitpick comments:
In `@dash/src/sml/masternode_list_entry/hash.rs`:
- Around line 6-10: The method calculate_entry_hash currently calls
self.consensus_encode_body(&mut writer).expect("encoding failed"), which uses
expect in library code; replace this by keeping the current behavior but add a
concise comment explaining why encoding to a Vec<u8> is effectively infallible
(the Write impl for Vec only fails on allocation exhaustion which would abort)
and that returning a Result was considered but omitted for API stability;
reference the calculate_entry_hash and consensus_encode_body symbols and state
that sha256d::Hash::hash(&writer) remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa716e82-9a61-4a6f-9dd8-5f0fb0b7d223
📒 Files selected for processing (3)
dash/src/sml/address.rsdash/src/sml/masternode_list_entry/hash.rsdash/src/sml/masternode_list_entry/mod.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #798 +/- ##
==========================================
+ Coverage 72.55% 72.70% +0.15%
==========================================
Files 322 322
Lines 71006 71402 +396
==========================================
+ Hits 51518 51913 +395
- Misses 19488 19489 +1
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MasternodeListEntry::calculate_entry_hashhashed the full network consensus serialization, including the leadingversion. Dash Core'sCSimplifiedMNListEntry::CalcHashusesCHashWriter(SER_GETHASH, ...), andSER_GETHASHdoes not setSER_NETWORK, so theSER_NETWORK-gated leadingnVersionis omitted from the hash pre-image. The two contexts therefore diverged for every entry.Extract the post-
versionbody of the wire encoder into a sharedconsensus_encode_bodyhelper.consensus_encodewritesversionthen the body (byte-identical to before), whilecalculate_entry_hashhashes the body alone, matching Core'sCalcHash. Verified against Core for both aversion1 and aversion2 Evo entry.Also fix
SocketAddrdecoding to useto_ipv4_mappedinstead ofto_ipv4, so an all-zero (::) service address round-trips to the same 16 bytes instead of acquiring an::ffff:mapped prefix. The prefix corruption changed the wire bytes and thus the entry hash for masternodes with an unset service.Summary by CodeRabbit
Bug Fixes
Tests