backport: bitcoin#21186, #21940, #21941, #22098, #22399, #22582, #22798, #22922, #23036, bitcoin-core/gui#420 #7024
Conversation
…essing 0829516 [refactor] Remove unused ForEachNodeThen() template (John Newbery) 09cc66c scripted-diff: rename address relay fields (John Newbery) 76568a3 [net processing] Move addr relay data and logic into net processing (John Newbery) caba7ae [net processing] Make RelayAddress() a member function of PeerManagerImpl (John Newbery) 86acc96 [net processing] Take NodeId instead of CNode* as originator for RelayAddress() (John Newbery) Pull request description: This continues the work of moving application layer data into net_processing, by moving all addr data into the new Peer object added in bitcoin#19607. For motivation, see bitcoin#19398. ACKs for top commit: laanwj: Code review ACK 0829516 mzumsande: ACK 0829516, reviewed the code and ran tests. sipa: utACK 0829516 hebasto: re-ACK 0829516 Tree-SHA512: efe0410fac288637f203eb37d1999910791e345872d37e1bd5cde50e25bb3cb1c369ab86b3a166ffd5e06ee72e4508aa2c46d658be6a54e20b4f220d2f57d0a6
…z test only once faf7623 fuzz: Call const member functions in addrman fuzz test only once (MarcoFalke) Pull request description: Logically based on bitcoin#21940 Currently the fuzz test may spend a long time generating random numbers:  Fix that by calling const member functions only once. Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34224 ACKs for top commit: practicalswift: cr ACK faf7623: touches only `src/test/fuzz/addrman.cpp` Tree-SHA512: 0fe9e0111eb1706fc39bd2f90d4b87a771882bada54c01e96d8e79c2afca2f1081139d5ab680285a81835cc5142e74ada422a181db34b01904975d1e167e64c2
fabf170 fuzz: Move CTxDestination fuzzing to script fuzz target (MarcoFalke) fa42800 fuzz: Simplify CTxDestination fuzzing in the script target (MarcoFalke) fab9986 fuzz: Improve ConsumeTxDestination (MarcoFalke) fa40c09 fuzz: Move ConsumeTxDestination to cpp file (MarcoFalke) Pull request description: ACKs for top commit: practicalswift: cr ACK fabf170 Tree-SHA512: afd2cf384d04a810c0c462c6d80849bd0fefd017d7acac877f64f2bffae3fc8d687701bc479e67a727a05f43431a17cb4ccaf09c6b3c68106562c94b7ed19250
Leftover missing const
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis pull request updates DNS seed handling and related tests and utilities. It adds a placeholder DNS seed for regtest, enforces that -forcednsseed cannot be true when -dnsseed is false, changes three default booleans to Sequence DiagramsequenceDiagram
autonumber
participant User
participant Node
participant DNS
participant AddrMan
participant Peers
User->>Node: start node (with args: -dnsseed / -forcednsseed / -connect)
Node->>Node: parse args & init parameters
Node->>Node: validate flags (-forcednsseed vs -dnsseed)
alt invalid flags
Node-->>User: InitError (invalid flag combination)
else valid
Node->>AddrMan: query address count
alt addrman < 1000
Node->>Node: schedule DNS seed query (11s delay)
else addrman >= 1000
Node->>Node: schedule DNS seed query (300s delay)
end
Node->>Peers: check outbound / block-relay connections
alt insufficient outbound peers
Node->>DNS: query DNS seeds (may use regtest dummySeed)
DNS-->>Node: return addresses
Node->>AddrMan: add addresses
AddrMan-->>Peers: provide addresses for connection attempts
else enough outbound peers
Node->>Node: skip DNS seeding
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (18)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used📓 Path-based instructions (2)src/**/*.{cpp,h,hpp,cc}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/functional/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-06-09T16:43:20.996ZApplied to files:
🧬 Code graph analysis (6)test/functional/p2p_dns_seeds.py (1)
test/functional/p2p_addrv2_relay.py (2)
test/functional/mempool_package_limits.py (3)
test/functional/rpc_net.py (1)
test/functional/rpc_signer.py (2)
test/functional/feature_config_args.py (1)
🪛 Ruff (0.14.6)test/functional/mempool_package_limits.py216-216: Loop control variable Rename unused (B007) 🔇 Additional comments (13)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
src/chainparams.cpp(1 hunks)src/init.cpp(1 hunks)src/net.h(1 hunks)src/qt/rpcconsole.cpp(2 hunks)src/qt/sendcoinsdialog.cpp(1 hunks)src/rpc/external_signer.cpp(1 hunks)src/test/fuzz/addrman.cpp(2 hunks)src/test/fuzz/connman.cpp(0 hunks)src/test/fuzz/integer.cpp(0 hunks)src/test/fuzz/key_io.cpp(0 hunks)src/test/fuzz/script.cpp(2 hunks)src/test/fuzz/util.cpp(2 hunks)src/test/fuzz/util.h(2 hunks)test/functional/feature_config_args.py(2 hunks)test/functional/mempool_package_limits.py(3 hunks)test/functional/p2p_addr_relay.py(3 hunks)test/functional/p2p_addrfetch.py(1 hunks)test/functional/p2p_addrv2_relay.py(3 hunks)test/functional/p2p_dns_seeds.py(1 hunks)test/functional/rpc_net.py(3 hunks)test/functional/rpc_signer.py(1 hunks)test/functional/test_framework/p2p.py(1 hunks)test/functional/test_runner.py(1 hunks)test/functional/tool_wallet.py(1 hunks)
💤 Files with no reviewable changes (3)
- src/test/fuzz/key_io.cpp
- src/test/fuzz/integer.cpp
- src/test/fuzz/connman.cpp
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/chainparams.cppsrc/qt/sendcoinsdialog.cppsrc/test/fuzz/util.hsrc/test/fuzz/script.cppsrc/test/fuzz/util.cppsrc/init.cppsrc/net.hsrc/qt/rpcconsole.cppsrc/test/fuzz/addrman.cppsrc/rpc/external_signer.cpp
src/qt/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
GUI implementation in src/qt/ must use Qt 5
Files:
src/qt/sendcoinsdialog.cppsrc/qt/rpcconsole.cpp
src/{test,wallet/test}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Files:
src/test/fuzz/util.hsrc/test/fuzz/script.cppsrc/test/fuzz/util.cppsrc/test/fuzz/addrman.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests in test/functional/ must be written in Python (minimum version specified in .python-version) and depend on dashd and dash-node
Files:
test/functional/p2p_addrfetch.pytest/functional/rpc_signer.pytest/functional/rpc_net.pytest/functional/p2p_addrv2_relay.pytest/functional/test_framework/p2p.pytest/functional/feature_config_args.pytest/functional/test_runner.pytest/functional/tool_wallet.pytest/functional/p2p_dns_seeds.pytest/functional/mempool_package_limits.pytest/functional/p2p_addr_relay.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
src/qt/sendcoinsdialog.cpp
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/qt/sendcoinsdialog.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/test/fuzz/script.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
test/functional/feature_config_args.py
🧬 Code graph analysis (9)
test/functional/p2p_addrfetch.py (3)
test/functional/test_framework/messages.py (1)
CAddress(258-381)test/functional/test_framework/p2p.py (1)
P2PInterface(492-770)src/protocol.h (1)
nServices(482-490)
src/test/fuzz/script.cpp (2)
src/test/fuzz/util.cpp (2)
ConsumeTxDestination(439-455)ConsumeTxDestination(439-439)src/script/descriptor.cpp (1)
EncodeDestination(664-664)
test/functional/rpc_signer.py (2)
test/functional/test_framework/util.py (1)
assert_raises_rpc_error(132-148)src/rpc/external_signer.cpp (2)
enumeratesigners(16-62)enumeratesigners(16-16)
test/functional/rpc_net.py (2)
test/functional/test_framework/p2p.py (1)
P2PInterface(492-770)test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/p2p_addrv2_relay.py (2)
test/functional/test_framework/p2p.py (1)
P2PInterface(492-770)src/protocol.h (1)
nServices(482-490)
test/functional/feature_config_args.py (2)
src/txmempool.h (1)
exists(788-792)test/functional/test_framework/test_node.py (1)
assert_debug_log(444-472)
src/init.cpp (2)
src/chainparams.cpp (13)
args(699-699)args(708-708)args(754-754)args(755-755)args(756-756)args(757-757)args(758-758)args(759-759)args(961-961)args(971-971)args(982-982)args(1005-1005)args(1006-1006)src/validation.cpp (3)
args(690-690)args(699-699)args(705-705)
test/functional/p2p_dns_seeds.py (3)
test/functional/test_framework/p2p.py (1)
P2PInterface(492-770)test/functional/feature_config_args.py (1)
set_test_params(15-20)test/functional/test_framework/test_node.py (2)
assert_debug_log(444-472)add_outbound_p2p_connection(710-772)
test/functional/mempool_package_limits.py (2)
test/functional/test_framework/messages.py (1)
tx_from_hex(230-232)test/functional/test_framework/wallet.py (1)
make_chain(346-364)
🪛 Ruff (0.14.6)
test/functional/mempool_package_limits.py
216-216: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (28)
test/functional/rpc_signer.py (1)
56-62: LGTM! Platform-aware error handling is correctly implemented.The change appropriately handles OS-specific error messages when attempting to execute a non-existent external signer script. The Windows and Unix error messages accurately reflect the underlying system calls (CreateProcess vs. execve), making the test portable across platforms.
src/test/fuzz/addrman.cpp (2)
282-288: LGTM: Const-correctness testing for read-only operations.The const reference wrapper correctly tests that
GetAddr,Select, andSizecan be invoked on a constAddrManinstance, which is appropriate for these read-only operations.
290-290: LGTM: Serialization through const reference.Consistent with the const-correctness pattern, serialization now operates on the const reference, which correctly reflects that serialization is a read-only operation.
test/functional/mempool_package_limits.py (3)
27-27: LGTM!The
DEFAULT_FEEimport is appropriately added to support fee calculations in the new test method.
52-52: LGTM!The new test is appropriately placed in the test sequence after the related
test_desc_count_limitstest.
197-244: Test logic is correct and well-structured.The test properly validates descendant count limits by:
- Creating M1 with 2 outputs
- Building a chain of 23 transactions (M2...M24) from one output
- Creating P1 and P2 from the other output
- Verifying that M1 exceeds descendant limits (23 in-mempool + 2 in-package = 26 total including M1)
- Confirming the package passes after clearing the mempool
Note: Unlike
test_desc_count_limits(line 123) and other similar tests, this test doesn't assert the mempool is empty at the start. This is fine since the preceding test clears the mempool, but addingassert_equal(0, node.getmempoolinfo()["size"])would improve test isolation if this matches upstream.src/init.cpp (1)
1226-1229: DNS seed flags consistency check is soundThe new guard cleanly rejects the contradictory configuration where
-forcednsseedis true while-dnsseedresolves to false after parameter interaction. This matches the intended semantics of the flags and integrates consistently with the surrounding InitError-based validations.src/qt/sendcoinsdialog.cpp (1)
229-239: Translator hint for hardware signer text is appropriateThe updated translator comment around the “Sign on device” label correctly clarifies that “device” refers to a hardware wallet and does not affect runtime behavior or translation keys.
src/qt/rpcconsole.cpp (1)
735-773: Context-menu translator comments are clearer with no behavioral changeThe added/expanded translator comments for “Copy address” and “Copy IP/Netmask” give better context to translators while leaving the actions and UI strings untouched.
src/chainparams.cpp (1)
857-859: Regtest dummy DNS seed aligns with expected behaviorAdding
vSeeds.clear();and a dummy"dummySeed.invalid."entry for regtest keeps real networks unaffected while letting DNS‑seed logic run in tests. Change looks correct and consistent with upstream behavior.test/functional/tool_wallet.py (1)
355-357: Platform-correct wallet path constructionUsing
os.path.join(self.nodes[0].datadir, 'regtest', 'wallets', 'todump2')should now match the node’s actual database path on all platforms, making the error assertion robust.src/rpc/external_signer.cpp (1)
24-32: RPC help schema now matches actual enumeratesigners resultDocumenting
signersas an array of objects withfingerprintandnamecorrectly reflects the existing implementation that already returns objects, so this is a safe documentation alignment.test/functional/feature_config_args.py (2)
185-198: Clarified DNS seed expectations and more robust log waitThe added comment and explicit
timeout=10onassert_debug_logmake the DNS‑seed/fixed‑seed behavior clearer and the test less flaky on slow CI nodes.
233-240: Second addrman/fixed-seeds wait uses consistent timeoutUsing
timeout=10here mirrors the earlier case and should reduce timing-related flakes while preserving the intended semantics of the test.src/net.h (1)
115-117: constexpr DNS seed defaults are safe and idiomaticSwitching the DNS seed defaults to
static constexpr boolpreserves semantics while making them usable in constant expressions; fully compatible with the stated C++20 toolchain.test/functional/test_runner.py (1)
150-163: Including p2p_dns_seeds.py in BASE_SCRIPTS is appropriateAdding
p2p_dns_seeds.pyto the base suite integrates the new DNS‑seed test into normal runs and satisfies the naming/prefix checks.test/functional/p2p_addrfetch.py (1)
11-19: Use of P2P_SERVICES for addr-fetch test address is consistentImporting
P2P_SERVICESand assigningADDR.nServices = P2P_SERVICESaligns this test with the framework’s standard service flag set, keeping behavior and expectations consistent across P2P tests.Also applies to: 23-27
test/functional/p2p_addrv2_relay.py (1)
15-18: Correct addrv2 size accounting for multi-bit P2P_SERVICESSwitching to
P2P_SERVICESand updatingcalc_addrv2_msg_sizeto add 3 bytes for the compactsize‑encoded services field matches the actual wire encoding, so the debug‑log size checks should now be accurate.Also applies to: 44-52, 61-76
test/functional/p2p_addr_relay.py (1)
11-27: Addr relay test updates correctly aligned with P2P_SERVICES
addr.nServicesis now set fromP2P_SERVICESand checked against2049, which matchesNODE_NETWORK | NODE_HEADERS_COMPRESSEDgiven the new definition intest_framework/p2p.py. The additionalmsg_veracksend inAddrReceiver.on_versionand the use ofassert_greater_than_or_equalin the rotation test are consistent with upstream behavior and the rest of this PR. No issues spotted.Also applies to: 48-55, 107-113, 403-417
test/functional/test_framework/p2p.py (1)
543-547: peer_accept_connection default services correctly updatedUsing
services=P2P_SERVICEShere brings inbound-side defaults in line withpeer_connectand the rest of the test framework. Becauseservicesis a keyword-only argument, it is not forwarded toP2PConnection.peer_accept_connection, so existing call sites that already passserviceskeep working without impacting the lower-level protocol object. Looks good.test/functional/rpc_net.py (1)
10-16: Service flag expectations in rpc_net tests are consistent with new P2P semanticsImporting
P2PInterfacefortest_service_flagsand explicitly setting(1 << 4) | (1 << 63)matches the expectedservicesnamesarrays, withP2P_V2only present under v2 transport. Intest_getnodeaddresses, assertingservices == 1for both IPv4 and IPv6 results correctly encodes that RPC-added peers carry just theNODE_NETWORKbit, decoupling the test from theNODE_NETWORKsymbol while staying semantically identical. No functional issues noted.Also applies to: 262-269, 271-313
test/functional/p2p_dns_seeds.py (1)
1-129: New DNS seed functional test matches upstream behavior and looks correctThe
P2PDNSSeedstest class thoroughly covers interactions between-connect,-dnsseed, and-forcednsseed, as well as the outbound vs block-relay-only and addrman-size–dependent delay paths. The use ofassert_debug_logfor specific messages, explicit-dnsseed=1where needed, and addrman population logic are consistent with the corresponding upstream Bitcoin Core test, with no Dash-specific regressions apparent.src/test/fuzz/util.cpp (2)
8-8: LGTM!The
pubkey.hinclude is appropriately added to support thePKHashtype used in the newConsumeTxDestinationfunction below.
439-455: No action needed. The assertion is correct for Dash Core.Dash Core explicitly excludes SegWit witness types from its CTxDestination variant, which contains exactly 3 types:
CNoDestination,PKHash(equivalent toCKeyID), andScriptHash(equivalent toCScriptID). The assertioncall_size == std::variant_size_v<CTxDestination>correctly validates that theCallOneOffunction exercises all 3 variant alternatives.src/test/fuzz/util.h (2)
103-113: LGTM!The change to return
call_sizefromCallOneOfis a clean enhancement that enables callers to verify they've provided the expected number of callables, useful for ensuring exhaustive variant coverage.
237-237: LGTM!Moving the
ConsumeTxDestinationdefinition to the.cppfile is appropriate given the new assertion logic and reduces header dependencies.src/test/fuzz/script.cpp (2)
10-13: LGTM!The new includes are correctly added to support
EncodeDestination,DecodeDestination,IsValidDestinationStringfromkey_io.handDescribeAddressfromrpc/util.h.
131-151: LGTM!The enhanced destination fuzzing comprehensively exercises the destination encoding/decoding round-trip, script generation, validity checks, and equality comparisons. The assertions correctly verify:
- Round-trip encoding idempotency (line 138)
- Script emptiness correlates with validity (line 142)
- String validity matches destination validity (line 144)
- Equal destinations produce equal derived values (lines 148-150)
This provides good coverage for destination-related code paths.
| def test_desc_count_limits_2(self): | ||
| """Create a Package with 24 transaction in mempool and 2 transaction in package: | ||
| M1 | ||
| ^ ^ | ||
| M2 ^ | ||
| . ^ | ||
| . ^ | ||
| . ^ | ||
| M24 ^ | ||
| ^ | ||
| P1 | ||
| ^ | ||
| P2 | ||
| P1 has M1 as a mempool ancestor, P2 has no in-mempool ancestors, but when | ||
| combined P2 has M1 as an ancestor and M1 exceeds descendant_limits(23 in-mempool | ||
| descendants + 2 in-package descendants, a total of 26 including itself). | ||
| """ |
There was a problem hiding this comment.
Minor typo in docstring.
Line 180: "24 transaction" should be "24 transactions" (plural).
def test_desc_count_limits_2(self):
- """Create a Package with 24 transaction in mempool and 2 transaction in package:
+ """Create a Package with 24 transactions in mempool and 2 transactions in package:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_desc_count_limits_2(self): | |
| """Create a Package with 24 transaction in mempool and 2 transaction in package: | |
| M1 | |
| ^ ^ | |
| M2 ^ | |
| . ^ | |
| . ^ | |
| . ^ | |
| M24 ^ | |
| ^ | |
| P1 | |
| ^ | |
| P2 | |
| P1 has M1 as a mempool ancestor, P2 has no in-mempool ancestors, but when | |
| combined P2 has M1 as an ancestor and M1 exceeds descendant_limits(23 in-mempool | |
| descendants + 2 in-package descendants, a total of 26 including itself). | |
| """ | |
| def test_desc_count_limits_2(self): | |
| """Create a Package with 24 transactions in mempool and 2 transactions in package: | |
| M1 | |
| ^ ^ | |
| M2 ^ | |
| . ^ | |
| . ^ | |
| . ^ | |
| M24 ^ | |
| ^ | |
| P1 | |
| ^ | |
| P2 | |
| P1 has M1 as a mempool ancestor, P2 has no in-mempool ancestors, but when | |
| combined P2 has M1 as an ancestor and M1 exceeds descendant_limits(23 in-mempool | |
| descendants + 2 in-package descendants, a total of 26 including itself). | |
| """ |
🤖 Prompt for AI Agents
In test/functional/mempool_package_limits.py around lines 179 to 195, the
docstring has a minor grammatical typo: change "24 transaction" to "24
transactions" so the sentence correctly uses the plural form.
| for i in range(23): # M2...M24 | ||
| (tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk) | ||
| txid = tx.rehash() | ||
| node.sendrawtransaction(txhex) |
There was a problem hiding this comment.
Unused loop variable flagged by linter.
The loop variable i is not used within the loop body. Per Ruff B007, rename to _ to indicate it's intentionally unused.
- for i in range(23): # M2...M24
+ for _ in range(23): # M2...M24
(tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk)
txid = tx.rehash()
node.sendrawtransaction(txhex)🧰 Tools
🪛 Ruff (0.14.6)
216-216: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🤖 Prompt for AI Agents
In test/functional/mempool_package_limits.py around lines 216 to 219, the
for-loop declares an unused loop variable `i`; rename `i` to `_` to satisfy the
linter rule (Ruff B007) indicating the loop index is intentionally unused, e.g.
change "for i in range(23):" to "for _ in range(23):".
fa0c194 cirrus: Enable tests on windows (MarcoFalke) fadecbd test: Fix tests on Windows (MarcoFalke) Pull request description: Only a cherry-picked list. `--extended` can be enabled in a follow-up. ACKs for top commit: hebasto: ACK fa0c194, tested locally on Windows 10 Pro 20H2 (build 19042.1165): Tree-SHA512: 47cfbcef7ce5fe0c62b77a1e45ace513c4f0109b1fcfaec94faf9e488fe9430d6ba0e852230021d432847eb1389a4e4b7cf39bf17f7b09bae36af3079e0d7399
fa7db1c [test] checks descendants limtis for second generation Package descendants (ritickgoenka) Pull request description: This PR adds a new functional test to test the new descendant limits for packages that were proposed in bitcoin#21800. ``` +----------------------+ | | | M1 | | ^ ^ | | M2 ^ | | . ^ | | . ^ | | . ^ | | . ^ | | M24 ^ | | ^ | | P1 | | ^ | | P2 | | | +----------------------+ ``` This test is for checking a transaction to fail its descendant count limits because of a combination of mempool descendants, package direct descendants, and package indirect descendants. In this test, P1 has M1 as a mempool ancestor, P2 has no in-mempool ancestors, but when combined P2 has M1 as an ancestor and M1 exceeds descendant_limits (23 in-mempool descendants + 2 in-package descendants, a total of 26 including itself) ACKs for top commit: ryanofsky: Code review ACK fa7db1c. Only were suggested changes since last review: simplifying test and dropping P3 transaction as John suggested, and adding assert_equal I suggested glozow: ACK fa7db1c jnewbery: ACK fa7db1c Tree-SHA512: d1eb993550ac8ce31cbe42e17c6522a213ede66970d5d9391f31a116477ab5889fefa6ff2df6ceadd63a28c1be1ad893b0e8e449247e9ade2ca61dc636508d68
5cc783f qt: ensure translator comments end in full stop (Jarol Rodriguez) Pull request description: This is a follow-up to dashpay#318 which addresses this [nit](bitcoin-core/gui#318 (comment)) by addressing it globally. This ensures that all GUI translator comments end in a full stop. If a comment does not end in a full stop, a translator may think that the rest of the comment is being cut off. While here, add a colon to the word "see" for any comments touched which point to look at a link. ACKs for top commit: hebasto: ACK 5cc783f, I have reviewed the code and it looks OK, I agree it can be merged. shaavan: Code Review ACK 5cc783f Tree-SHA512: 67a1d56175c974e0af9b460fa44163f7ce139a7b81cfaf8ed2c0e7fb6d5120957c3135d96010aeb6229689468e36673fe9571b5a8c3e1c07e047aba1bd563444
BACKPORT NOTE: added: - changes in `rpc/external_signer.cpp` still missing: - field `signet_challenge` in rpc/mining.cpp fa10fbc doc: Fix RPC result documentation (MarcoFalke) Pull request description: Fix: * Incorrectly named fields * Add missing ones * Add missing optional flag ACKs for top commit: fanquake: ACK fa10fbc Tree-SHA512: 2b302e6f7ac8253a55882bc032ddda1932b728abddd12b0adb5fba814b12fb998a67b91e6dd124ebbe0ac16dccdace01332ade01c1dc00a3768647118dd3a2d2
…tant in functional tests b69a106 test: use test_framework.p2p P2P_SERVICES in functional tests (Jon Atack) Pull request description: `P2P_SERVICES` is defined in `test/functional/test_framework/p2p.py`, so we can use it as a single definition for our functional tests. It may also be a tiny bit more efficient to use the constant rather than calculating `NODE_NETWORK | NODE_WITNESS` every time we need it in the tests. ACKs for top commit: laanwj: Code review ACK b69a106 klementtan: crACK b69a106 fanquake: ACK b69a106 - didn't look at the formatting changes. Tree-SHA512: f83e593663a69182986325d9ba2b4b787b87896d6648973f4f802f191a2573201b9e7d7e10e69662ef1965fa63268845726ed1aa5742a2e38dcccf4aebc6a961
…es dns probe connections
82b6f89 [style] Small style improvements to DNS parameters (Amiti Uttarwar) 4c89e24 [test] Test the delay before querying DNS seeds (Amiti Uttarwar) 6395c8e [test] Test the interactions between -forcednsseed and -dnsseed (Amiti Uttarwar) 6f6b7df [init] Disallow starting up with conflicting paramters for -dnsseed and -forcednsseed (Amiti Uttarwar) 26d0ffe [test] Test -forcednsseed causes querying DNS seeds (Amiti Uttarwar) 3585145 [test] Test the interactions between -connect and -dnsseed (Amiti Uttarwar) 75c05af [test] Test logic to query DNS seeds with block-relay-only connections (Amiti Uttarwar) 9c08719 [test] Introduce test logic to query DNS seeds (Amiti Uttarwar) Pull request description: This PR adds a DNS seed to the regtest chain params to enable testing the DNS seed querying logic of `CConnman::ThreadDNSAddressSeed` and relevant startup parameters. Adds coverage for the changes in bitcoin#22013 (and then some). The main behavioral change to bitcoind is that this PR disallows starting up with conflicting parameters for `-dnsseed` and `-forcednsseed`. The tests include: * parameter interactions of different combinations of `-connect`, `-dnsseed` and `-forcednsseed` * the delay before querying DNS seeds depending on how many addresses are in the addrman * the behavior of `-forcednsseed` * skipping DNS querying if we have outbound full relay connections & not block-relay-only connections Huge props to mzumsande for identifying the timing technique for testing successful connections before running `ThreadDNSAddressSeed` 🙌🏽 ACKs for top commit: mzumsande: ACK 82b6f89 jnewbery: reACK 82b6f89 Tree-SHA512: 9f0c29bfbf99426727e79c0a25606ae09deab91a92e3c5cee7f84c3ca7503a8ac9ab85a85c51841d40b164ef8c991326070f0b2f41d075fb7985df26f6e95d6d
3 participants
What was done?
Some regular backports, mostly from Bitcoin Core v23 that has been missing by various reasons
Also fixed conditions for DNS-seed requesting, as follow-up for bitcoin#19316
How Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Changes of RPC in src/rpc/external_signer.cpp are not breaking changes because it has not been released yet.
Checklist: