backport: Merge bitcoin/bitcoin#28831, 28966#7177
Conversation
|
0042783 to
30eb354
Compare
WalkthroughThis pull request makes three distinct changes across test and configuration files. The first change eliminates randomness in a test perturbation mechanism by replacing random offset selection and variable-length data writes with fixed values. The second modifies an addrman test to introduce time-based mocking, setting a specific timestamp before adding peer addresses and validating the results. The third adds a new UBSan suppression for an implicit integer sign-change in the CBlockPolicyEstimator::processBlockTx function. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean backport of three upstream Bitcoin test-only PRs (bitcoin#28671, bitcoin#28831, bitcoin#28966). No blocking issues, no missing prerequisites. Minor observation: stored addr_time field is never asserted in the test.
Reviewed commit: eb5e2a0
|
✅ Review complete (commit aac5a77) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, low-risk backport PR pulling three test-only Bitcoin Core changes (bitcoin#28831, bitcoin#28671, bitcoin#28966). Verified: the cherry-pick of bitcoin#28671 only includes the addpeeraddress mocktime hunk, not the second hunk that asserts on getrawaddrman, because Dash has not backported the prerequisite bitcoin#28523 (no getrawaddrman in source tree). This leaves self.addr_time set but never read — non-blocking and a faithful partial backport.
Reviewed commit: eb5e2a0
🟡 1 suggestion(s)
🤖 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 `test/functional/rpc_net.py`:
- [SUGGESTION] lines 356-357: Partial backport of bitcoin#28671 — getrawaddrman hunk dropped (prereq bitcoin#28523 not backported)
Upstream bitcoin#28671 contains two hunks in test/functional/rpc_net.py: (1) the addpeeraddress mocktime setup carried over here, and (2) an `assert_equal(result['time'], self.addr_time)` check inside the `getrawaddrman`-driven loop. Dash does not have the `getrawaddrman` RPC (introduced upstream by bitcoin#28523, not backported — verified via grep: zero matches in src/ and test/). Only the first hunk applies, so `self.addr_time` is assigned at line 356 but never consumed elsewhere in the file. Non-blocking; the mocktime call still pins addrman timestamps benignly. Consider either dropping the `self.` prefix (use a local), or leaving a comment so a future bitcoin#28523 backport completes the pair.
| self.addr_time = int(time.time()) | ||
| node.setmocktime(self.addr_time) |
There was a problem hiding this comment.
🟡 Suggestion: Partial backport of bitcoin#28671 — getrawaddrman hunk dropped (prereq bitcoin#28523 not backported)
Upstream bitcoin#28671 contains two hunks in test/functional/rpc_net.py: (1) the addpeeraddress mocktime setup carried over here, and (2) an assert_equal(result['time'], self.addr_time) check inside the getrawaddrman-driven loop. Dash does not have the getrawaddrman RPC (introduced upstream by bitcoin#28523, not backported — verified via grep: zero matches in src/ and test/). Only the first hunk applies, so self.addr_time is assigned at line 356 but never consumed elsewhere in the file. Non-blocking; the mocktime call still pins addrman timestamps benignly. Consider either dropping the self. prefix (use a local), or leaving a comment so a future bitcoin#28523 backport completes the pair.
source: ['claude-general', 'claude-backport-reviewer', 'codex-backport-reviewer']
🤖 Fix this 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 `test/functional/rpc_net.py`:
- [SUGGESTION] lines 356-357: Partial backport of bitcoin#28671 — getrawaddrman hunk dropped (prereq bitcoin#28523 not backported)
Upstream bitcoin#28671 contains two hunks in test/functional/rpc_net.py: (1) the addpeeraddress mocktime setup carried over here, and (2) an `assert_equal(result['time'], self.addr_time)` check inside the `getrawaddrman`-driven loop. Dash does not have the `getrawaddrman` RPC (introduced upstream by bitcoin#28523, not backported — verified via grep: zero matches in src/ and test/). Only the first hunk applies, so `self.addr_time` is assigned at line 356 but never consumed elsewhere in the file. Non-blocking; the mocktime call still pins addrman timestamps benignly. Consider either dropping the `self.` prefix (use a local), or leaving a comment so a future bitcoin#28523 backport completes the pair.
44445ae test: Avoid intermittent failures in feature_init (MarcoFalke) Pull request description: The code not only modifies block dat files, but also leveldb files, which may be of smaller size. Such corruption may not force leveldb to abort, according to the intermittent test failures. Fix the intermittent test failures by reverting bitcoin@5ab6419 . ACKs for top commit: kevkevinpal: lgtm ACK [44445ae](bitcoin@44445ae) fjahr: ACK 44445ae theStack: ACK 44445ae Tree-SHA512: 8084e4aeb8a976c1706a1898d7854c55d0c4ec7b5a08f65f97ffc173c935f9b0e0c1caef7be1538a458e4c018f7bd1948173349ec76ca48bc4013a63f284bb0f
…BlockTx suppression fa9dc92 test: Add missing CBlockPolicyEstimator::processBlockTx suppression (MarcoFalke) Pull request description: Fixes bitcoin#28865 (comment) ``` # FUZZ=policy_estimator UBSAN_OPTIONS="suppressions=/root/fuzz_dir/scratch/fuzz_gen/code/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./src/test/fuzz/fuzz /tmp/crash-154b42214e70781a9c1ad72d3f2693913dcf8c06 ... policy/fees.cpp:632:27: runtime error: implicit conversion from type 'unsigned int' of value 4294574080 (32-bit, unsigned) to type 'int' changed the value to -393216 (32-bit, signed) #0 0x55cbbe10daee in CBlockPolicyEstimator::processBlockTx(unsigned int, CTxMemPoolEntry const*) src/policy/fees.cpp:632:27 #1 0x55cbbe10e361 in CBlockPolicyEstimator::processBlock(unsigned int, std::vector<CTxMemPoolEntry const*, std::allocator<CTxMemPoolEntry const*>>&) src/policy/fees.cpp:680:13 #2 0x55cbbd84af48 in policy_estimator_fuzz_target(Span<unsigned char const>)::$_1::operator()() const src/test/fuzz/policy_estimator.cpp:69:40 #3 0x55cbbd84af48 in unsigned long CallOneOf<policy_estimator_fuzz_target(Span<unsigned char const>)::$_0, policy_estimator_fuzz_target(Span<unsigned char const>)::$_1, policy_estimator_fuzz_target(Span<unsigned char const>)::$_2, policy_estimator_fuzz_target(Span<unsigned char const>)::$_3>(FuzzedDataProvider&, policy_estimator_fuzz_target(Span<unsigned char const>)::$_0, policy_estimator_fuzz_target(Span<unsigned char const>)::$_1, policy_estimator_fuzz_target(Span<unsigned char const>)::$_2, policy_estimator_fuzz_target(Span<unsigned char const>)::$_3) src/./test/fuzz/util.h:43:27 #4 0x55cbbd84af48 in policy_estimator_fuzz_target(Span<unsigned char const>) src/test/fuzz/policy_estimator.cpp:38:9 #5 0x55cbbda1cc18 in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9 #6 0x55cbbda1cc18 in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5 #7 0x55cbbd26a944 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x190e944) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) #8 0x55cbbd253916 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18f7916) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) #9 0x55cbbd25945a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18fd45a) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) #10 0x55cbbd284026 in main (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x1928026) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) #11 0x7fe4aa8280cf (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) #12 0x7fe4aa828188 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) #13 0x55cbbd24e494 in _start (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18f2494) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change policy/fees.cpp:632:27 in ``` ``` # base64 /tmp/crash-154b42214e70781a9c1ad72d3f2693913dcf8c06 AQEAAAAAADkFlVwAAQEAAAAAADkFlZVcACTDSSsP3746IAZrH48khwMAAQEB/QEALQAACwAAAAAA FgAAAAAAAQAABgAAAAAAAAAAAAAAAAAAACcQAAAAAAAAAAAAAAAAAAAAAAD6AAAAOQWVXAABAQAA AAAAOQWVlVwAIMNJKw/fvjogBmsfjySHAwABAQH9AQAtAAALAAAAAAAAAAABAAAGAAAAAAAAAAAA AAAAAAAAJxAAAAAAAAAAAAAAAAAAAAAAAPr/AAAAAAAAAAAAAAQAAAAA/wAAAAAAAAAAAAAEAAAA AAEBAeAIAVwBXAAA/jbSBvwBKABSKBwBYgEB2wAEkvXInHYAAAAAAAAAvgAAAAAA/9//6v8e/xIk MgAlAiUAOw== ACKs for top commit: fanquake: ACK fa9dc92 dergoegge: utACK fa9dc92 Tree-SHA512: 3898c17c928ecc2bcc8c7086359e9ae00da2197b4d8e10c7bf6d12415326c9bca3ef6e1d8d3b83172ccfa604ce7e7371415262ba705225f9ea4da8b1a7eb0306
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review of rebase eb5e2a0→aac5a772. Prior partial-backport concern in test/functional/rpc_net.py is fully resolved — the bitcoin#28671 cherry-pick was dropped entirely in the rebase, removing the orphaned addr_time/mocktime hunk. The current head contains only two clean, self-contained test-only backports (28831, 28966) with no Dash subsystem impact. Only remaining observation is a PR-metadata mismatch: title still advertises 28671 even though it is no longer included.
💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Automated code review.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Review-requested same-SHA rerun against pinned head aac5a77. The prior review's only remaining observation — that the PR title still referenced bitcoin#28671 while the code contained only bitcoin#28831 and bitcoin#28966 — is resolved: the current title backport: Merge bitcoin/bitcoin#28831, 28966 accurately matches the two-commit stack (3937cf4 backporting bitcoin#28831 and aac5a77 backporting bitcoin#28966). Both backports are test-only/sanitizer-suppression changes with no missing prerequisites, upstream metadata is preserved, and all six review agents (claude/codex general, commit-history, and backport-reviewer) agree there are no actionable findings. No new issues introduced.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Fresh review-requested same-SHA follow-up for pinned head aac5a77. The PR head, title, and two-commit stack are unchanged since the automated same-SHA rerun completed earlier today; that six-agent run (claude/codex general, dash-core-commit-history, and backport-reviewer, with verifier) found no actionable issues. Current title backport: Merge bitcoin/bitcoin#28831, 28966 accurately matches the two test-only backports (3937cf4 for bitcoin#28831 and aac5a77 for bitcoin#28966); no missing prerequisites or Dash-specific regressions were identified.
Bitcoin Backporting