backport: bitcoin#16807, #21825, #22257, #23577, #24259, #24420, #24339, #24441, #24735, #25125, #25171 - #7492
backport: bitcoin#16807, #21825, #22257, #23577, #24259, #24420, #24339, #24441, #24735, #25125, #25171#7492knst wants to merge 11 commits into
Conversation
142e2da net: add I2P seeds to chainparamsseeds (Jon Atack) e01f173 contrib: add a few I2P seed nodes (Jon Atack) ea269c7 contrib: parse I2P addresses in generate-seeds.py (Jon Atack) Pull request description: Follow-up to bitcoin#21560 that updated the fixed seeds infra for BIP155 addresses and then added Tor v3 ones: - Update contrib/generate-seeds.py to parse I2P addresses - Add a few I2P nodes to contrib/seeds/nodes_main.txt - Run generate-seeds.py and add the I2P seeds to chainparamsseeds.h Reviewers, see contrib/seeds/README.md for more info and feel free to use the following CLI one-liner to check for and propose additional seeds for contrib/seeds/nodes_main.txt. You can also see how many I2P peers your node knows with cli -addrinfo. ```rake bitcoin-cli getnodeaddresses 0 | jq '.[] | (select(.address | contains(".b32.i2p"))) | .address' | sort ``` I verified the I2P addresses are correctly BIP155-serialized/deserialized by building with all seeds removed from chainparamsseeds.h except those added here, restarting with `-datadir=newdir -dnsseed=0` and running rpc ` getnodeaddresses 0` that initially returns only the new I2P addresses. ACKs for top commit: laanwj: ACK 142e2da vasild: ACK 142e2da Tree-SHA512: 040576012d5f1f034e2bd566ad654a6fdfd8ff7f6b12fa40c9fda1e948ebf8417fcea64cfc14938a41439370aa4669bab3e97274f9d4f9a6906fa9520afa9cf8 Co-authored-by: W. J. van der Laan <laanwj@protonmail.com>
|
⛔ Blockers found — Sonnet deferred (commit 1f1bacd) |
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (11)
WalkthroughThe change expands address decoding diagnostics and adds Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant validateaddress
participant DecodeDestination
participant LocateErrors
validateaddress->>DecodeDestination: decode address and collect error locations
DecodeDestination->>LocateErrors: locate Bech32 errors
LocateErrors-->>DecodeDestination: error text and indices
DecodeDestination-->>validateaddress: error and error_locations
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@contrib/seeds/generate-seeds.py`:
- Line 19: Update the I2P seed format example in the relevant documentation or
generator template from “<i2p>.b32.i2p:<port>” to “<i2p>.b32.i2p:0”, explicitly
reflecting that only port 0 is valid for I2P addresses.
- Around line 56-57: Update the address parsing branch in the seed generator to
recognize the .b32.i2p suffix case-insensitively, matching the core parser’s
behavior and accepting uppercase variants such as .b32.I2P while preserving the
existing base32 decoding logic.
In `@doc/bips.md`:
- Around line 41-42: Update the BIP 157/158 entry so the PR references use one
balanced parenthetical group around both links, removing the extra opening
wrapper before PR `#14121` while preserving all link text and URLs.
In `@test/functional/rpc_verifyislock.py`:
- Line 6: Remove the unused CTransaction symbol from the import statement in
rpc_verifyislock.py, while retaining tx_from_hex, hash256, ser_compact_size, and
ser_string.
🪄 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 Plus
Run ID: dc0de3bd-8e71-44f6-a9bc-178dbd1237c9
📒 Files selected for processing (18)
ci/test/00_setup_env_native_valgrind.shci/test/00_setup_env_s390x.shcontrib/seeds/generate-seeds.pycontrib/valgrind.suppdoc/bips.mddoc/release-notes-16807.mdsrc/key_io.cppsrc/key_io.hsrc/rpc/blockchain.cppsrc/rpc/output_script.cppsrc/test/fuzz/script_format.cppsrc/test/versionbits_tests.cppsrc/wallet/rpc/coins.cppsrc/wallet/rpc/transactions.cpptest/functional/interface_zmq_dash.pytest/functional/rpc_invalid_address_message.pytest/functional/rpc_verifyislock.pytest/functional/wallet_listreceivedby.py
| <ip>:<port> | ||
| [<ipv6>]:<port> | ||
| <onion>.onion:<port> | ||
| <i2p>.b32.i2p:<port> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document the I2P port restriction.
<port> implies that arbitrary ports are valid, but I2P addresses are accepted only with port 0; non-zero ports are rejected by src/test/evo_netinfo_tests.cpp. Document this as <i2p>.b32.i2p:0 to avoid generating invalid seed entries.
🤖 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 `@contrib/seeds/generate-seeds.py` at line 19, Update the I2P seed format
example in the relevant documentation or generator template from
“<i2p>.b32.i2p:<port>” to “<i2p>.b32.i2p:0”, explicitly reflecting that only
port 0 is valid for I2P addresses.
| elif addr.endswith('.b32.i2p'): | ||
| vchAddr = b32decode(addr[0:-8] + '====', True) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match the core parser’s case-insensitive suffix handling.
This rejects valid addresses such as ...b32.I2P, while src/netaddress.cpp lowercases the suffix and src/test/net_tests.cpp explicitly covers uppercase I2P addresses.
Proposed fix
- elif addr.endswith('.b32.i2p'):
+ elif addr.lower().endswith('.b32.i2p'):📝 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.
| elif addr.endswith('.b32.i2p'): | |
| vchAddr = b32decode(addr[0:-8] + '====', True) | |
| elif addr.lower().endswith('.b32.i2p'): | |
| vchAddr = b32decode(addr[0:-8] + '====', True) |
🤖 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 `@contrib/seeds/generate-seeds.py` around lines 56 - 57, Update the address
parsing branch in the seed generator to recognize the .b32.i2p suffix
case-insensitively, matching the core parser’s behavior and accepting uppercase
variants such as .b32.I2P while preserving the existing base32 decoding logic.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e23d643f81
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
e5bf514 to
07d7f06
Compare
88cc481 Modify copyright header on Bech32 code (Samuel Dobson) 5599813 Add lots of comments to Bech32 (Samuel Dobson) 2eb5792 Add release notes for validateaddress Bech32 error detection (MeshCollider) 42d6a02 Refactor and add more tests for validateaddress (Samuel Dobson) c4979f7 Add boost tests for bech32 error detection (MeshCollider) 02a7bde Add error_locations to validateaddress RPC (Samuel Dobson) b62b67e Add Bech32 error location function (Samuel Dobson) 0b06e72 More detailed error checking for base58 addresses (Samuel Dobson) Pull request description: Addresses (partially) bitcoin#16779 - no GUI change in this PR Adds a LocateError function the bech32 library, which is then called by `validateaddress` RPC, (and then eventually from a GUI tool too, future work). I think modifying validateaddress is nicer than adding a separate RPC for this. Includes tests. Based on https://github.com/sipa/bech32/blob/master/ecc/javascript/bech32_ecc.js Credit to sipa for that code ACKs for top commit: laanwj: Code review and manually tested ACK 88cc481 ryanofsky: Code review ACK 88cc481 with caveat that I only checked the new `LocateErrors` code to try to verify it didn't have unsafe or unexpected operations or loop forever or crash. Did not try to verify behavior corresponds to the spec. In the worst case bugs here should just affect error messages not actual decoding of addresses so this seemed ok. w0xlt: tACK 88cc481 Tree-SHA512: 9c7fe9745bc7527f80a30bd4c1e3034e16b96a02cc7f6c268f91bfad08a6965a8064fe44230aa3f87e4fa3c938f662ff4446bc682c83cb48c1a3f95cf4186688 Dash note: this is the remaining, Dash-side part of the backport. The Bech32 library part (src/bech32.{cpp,h}, src/test/bech32_tests.cpp) was backported earlier in ce6d7cb, already including the bitcoin#23577 follow-ups (bd1186e), so bech32::LocateErrors is used here in its final pair-returning form instead of the out-arg form this PR introduced upstream. Dash L1 has no Bech32 addresses: a string using the Bech32m HRP ("dash" on mainnet, "tdash" on the test chains) can only be a DIP-18 Dash Platform address, which never encodes an L1 destination. The upstream witness version/size checks are therefore replaced by DecodePlatformDestination(), so that a well-formed Platform address, or a Bech32m string with a broken DIP-18 payload, is reported with a specific reason rather than an empty error (validateaddress requires isvalid == error.empty()). The functional test drops the SegWit-only cases for the same reason and covers the DIP-18 equivalents instead; its Bech32m constants are built from the DIP-0018 test vectors with the helpers in test_framework/segwit_addr.py. Co-authored-by: W. J. van der Laan <laanwj@protonmail.com>
a4fe701 Make Bech32 LocateErrors return error list rather than using out-arg (Samuel Dobson) 2fa4fd1 Use std::iota instead of manually pushing range (Samuel Dobson) 405c96f Use bounds-checked array lookups in Bech32 error detection code (Samuel Dobson) 28d9c28 Simplify encoding of e in GF(1024) tables to (1,0) (Samuel Dobson) 14358a0 Replace GF1024 tables and syndrome constants with compile-time generated constexprs. (Samuel Dobson) 63f7b69 Update release note for bech32 error detection (Samuel Dobson) c8b9a22 Report encoding type in bech32 error message (Samuel Dobson) 92f0caf Improve Bech32 boost tests (Samuel Dobson) bb4d3e9 Address review comments for Bech32 error validation (Samuel Dobson) Pull request description: A number of follow-ups and improvements to the bech32 error location code, introduced in bitcoin#16807. Notably, this removes the hardcoded GF1024 tables in favour of constexpr table generation. ACKs for top commit: laanwj: Re-ACK a4fe701 Tree-SHA512: 6312373c20ebd6636f5797304876fa0d70fa777de2f6c507245f51a652b3d1224ebc55b236c9e11e6956c1e88e65faadab51d53587078efccb451455aa2e2276 Dash note: this is the remaining, Dash-side part of the backport; the src/bech32.{cpp,h} and src/test/bech32_tests.cpp changes were backported earlier in bd1186e. The "Make Bech32 LocateErrors return error list rather than using out-arg" hunk for src/key_io.cpp is not needed either, because Dash never had the intermediate out-arg API. "Not a valid Bech32 or Base58 encoding" is spelled "Not a valid Bech32m or Base58 encoding" here: Bech32m (DIP-18 Platform addresses) is the only Bech32-family encoding Dash accepts for addresses. Co-authored-by: W. J. van der Laan <laanwj@protonmail.com>
fa4b619 test: Remove unused valgrind suppressions (MarcoFalke) faccb2d test: Exclude broken feature_init for now (MarcoFalke) fa086d8 test: Properly skip feature_syscall_sandbox in valgrind (MarcoFalke) Pull request description: ACKs for top commit: fanquake: ACK fa4b619 Tree-SHA512: 5be1a8f288182d386531a033ae7258f753dd655dfa1746a52b65622a0359c2b7143a25b49c0747538308eed606a691847d2f59a5a0382b7751b8de7172adf0d3 Co-authored-by: fanquake <fanquake@gmail.com>
03bc08e doc: Mention missing BIP157 in bips.md (laanwj) e97e3de doc: Update bips.md for 23.x (laanwj) Pull request description: As far as I know, there have been no new bips implemented in this major release. Update `bips.md` accordingly. (if there are, please post below) ACKs for top commit: jonatack: ACK 03bc08e prayank23: ACK bitcoin@03bc08e Tree-SHA512: d671c37d1aab9f700f9688dabec056acfd2503c83bd3e1612ed1cee4ba92b99db002e34b6f1100b98543fe60f8b757ea2bedcc2ff020243cf30e27b4dd73d04c Co-authored-by: fanquake <fanquake@gmail.com>
bbbbeaf fuzz: Limit script_format to 100kB (MarcoFalke) Pull request description: The target is still one of the slowest ones, but doesn't seem incredibly important. Especially for sizes larger than the standard tx size. Fix that by limiting the script size. ACKs for top commit: fanquake: ACK bbbbeaf Tree-SHA512: b6cf7248753909ef2f21d8824f187e7c05732dd3b99619c0067f862f3c2b0f9a87779d4ddbbd3a7a4bae5c794280e2f0a223bf835d6bc6ccaba01817d69479a2 Co-authored-by: fanquake <fanquake@gmail.com>
BACKPORT NOTE: Some changes are missing because they are superseeded by bitcoin#25662 Affected files: - ci/test/00_setup_env_native_fuzz_with_valgrind.sh - ci/test/00_setup_env_native_valgrind.sh 15893a0 supp: remove Boost Valgrind suppression (fanquake) b0740fd ci: use DWARF-4 for Valgrind CI job (fanquake) Pull request description: [clang-14 defaults to using DWARF-5](https://releases.llvm.org/14.0.0/tools/clang/docs/ReleaseNotes.html#dwarf-support-in-clang), which breaks vlagrinds (3.18) ability to parse debug info. Valgrind [claims to support DWARF-5](https://valgrind.org/docs/manual/dist.news.html) from version 3.18 onwards, but maybe that only works when compiling with GCC. Explicitly use DWARF-4 for now. Note that from 11.0 [GCC also defaults to using DWARF-5](https://www.gnu.org/software/gcc/gcc-11/changes.html). Also remove a Boost related suppression. Top commit has no ACKs. Tree-SHA512: e4f476170ac5ccbb43d26e990b24753bda3985b2ac5c8a32e74d2d1d64d1b3a2d80a90fbab345f0a9e404eac7fbd783c20147379208e615d526657e8a57890ca Co-authored-by: MarcoFalke <falke.marco@gmail.com>
fae3200 test: Slim down versionbits_tests.cpp (MacroFake) Pull request description: Seems confusing to spin up a full chainman that isn't even used. Fix that by only spinning up logging. Also, remove the chainman include and comment. ACKs for top commit: fanquake: ACK fae3200 Tree-SHA512: 35261e9116c0c276f807453db3d635d83916ec2ffd99cf5641f8732736a30a542213096dcec550ef4522d97b3cafe384fdc6068138bc0b577c66fa61256719f8 Co-authored-by: fanquake <fanquake@gmail.com>
…base` logic a4703ce doc: add release notes about removal of the `deprecatedrpc=exclude_coinbase` (Sebastian Falbesoner) ef0aa74 rpc: wallet: remove `-deprecatedrpc=exclude_coinbase` logic (Sebastian Falbesoner) Pull request description: Including coinbase transactions in `receivedby` RPCs and adding the `-deprecatedrpc=exclude_coinbase` was done in PR bitcoin#14707 (released in v23.0). For the next release v24.0, this configuration option can be removed. ACKs for top commit: fanquake: ACK a4703ce Tree-SHA512: 97cd4e78501e64f678c78d2ebb5be5376688c023e34fced71dd24e432d27aa31a74b5483545f49ba0bdf48656d8b8b7bee74e3db26cf6daf112613f1caa4dfa4 Co-authored-by: MacroFake <falke.marco@gmail.com>
…utput types BACKPORT NOTE: has been done previously; applied missing changes in src/rpc/blockchain.cpp c821ab8 Use `GetAllOutputTypes` in `getblock` RPC function (Kiminuo) d970a85 Move `GetAllOutputTypes` function from `rpc/rawtransaction.cpp` to `rpc/util.{h|cpp}` (Kiminuo) Pull request description: This PR attempts to replicate https://github.com/bitcoin/bitcoin/blob/0ccf9b2e5594581deef2f60174c3651a57f93b64/src/rpc/rawtransaction.cpp#L547 to one other place (at the moment) so that users have better idea what RPC methods can actually return. I created this PR as a follow-up to the idea mentioned here bitcoin#23320 (comment) (resolved). ACKs for top commit: kristapsk: re-ACK c821ab8 Tree-SHA512: 5ff66a41ad7c43ec769f4a99933d2d070feea7c617286d94b6f9bfa1a2547a42211915778210a89074ad4b14d99f34852cc6871efed5e6f1e2ffedd40d669386 Co-authored-by: MarcoFalke <falke.marco@gmail.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The backport is generally well adapted, but the bitcoin#24259 Valgrind change is incomplete. It removes Ubuntu-20.04-specific suppressions while leaving the Valgrind job on Ubuntu 20.04, which can cause Valgrind reports to fail CI.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 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 `ci/test/00_setup_env_native_valgrind.sh`:
- [BLOCKING] ci/test/00_setup_env_native_valgrind.sh:8-9: Missing prerequisite hunk: bitcoin#24259's Ubuntu 22.04 Valgrind image
Upstream commit fa4b61911d54840e9a24bfcabafec159f013ee9a changes this task's image from `ubuntu:20.04` to `ubuntu:22.04` in the same commit that removes the `wcsnrtombs` and Boost suppressions. Dash applied the suppression removals and changed `contrib/valgrind.supp` to document Ubuntu 22.04 as the tested environment, but omitted the image override. `ci/dash/matrix.sh` sources `ci/test/00_setup_env.sh` first, so this task still inherits its `ubuntu:20.04` default. The job therefore runs Ubuntu 20.04 system libraries without their corresponding suppressions, and Valgrind's `--error-exitcode=1` can turn those reports into CI failures. Add the upstream task-specific Ubuntu 22.04 override.
| @@ -9,7 +9,7 @@ export LC_ALL=C.UTF-8 | |||
| export PACKAGES="valgrind clang llvm libclang-rt-dev python3-zmq libevent-dev bsdmainutils libboost-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev" | |||
There was a problem hiding this comment.
🔴 Blocking: Missing prerequisite hunk: bitcoin#24259's Ubuntu 22.04 Valgrind image
Upstream commit fa4b619 changes this task's image from ubuntu:20.04 to ubuntu:22.04 in the same commit that removes the wcsnrtombs and Boost suppressions. Dash applied the suppression removals and changed contrib/valgrind.supp to document Ubuntu 22.04 as the tested environment, but omitted the image override. ci/dash/matrix.sh sources ci/test/00_setup_env.sh first, so this task still inherits its ubuntu:20.04 default. The job therefore runs Ubuntu 20.04 system libraries without their corresponding suppressions, and Valgrind's --error-exitcode=1 can turn those reports into CI failures. Add the upstream task-specific Ubuntu 22.04 override.
| export PACKAGES="valgrind clang llvm libclang-rt-dev python3-zmq libevent-dev bsdmainutils libboost-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev" | |
| export DOCKER_NAME_TAG="ubuntu:22.04" | |
| export PACKAGES="valgrind clang llvm libclang-rt-dev python3-zmq libevent-dev bsdmainutils libboost-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev" |
source: ['codex']
What was done?
Regular backports from Bitcoin Core v22, v23, v24
How Has This Been Tested?
Run unit & functional tests
Breaking Changes
remove
-deprecatedrpc=exclude_coinbaselogicThe
validateaddressRPC now returns anerror_locationsarray for invalidaddresses, with the indices of invalid character locations in the address (if
known). For example, this will attempt to locate up to two Bech32 errors, and
return their locations if successful. Success and correctness are only guaranteed
if fewer than two substitution errors have been made.
The error message returned in the
errorfield now also returns more specificerrors when decoding fails.
Checklist: