backport: Merge bitcoin#30257, 30264, 29607 , 30047, 3006, 27905#7296
backport: Merge bitcoin#30257, 30264, 29607 , 30047, 3006, 27905#7296vijaydasmp wants to merge 6 commits into
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
vijaydasmp
changed the title
vijaydasmp
changed the title
vijaydasmp
changed the title
vijaydasmp
changed the title
vijaydasmp
changed the title
fa780e1 build: Remove --enable-gprof (MarcoFalke) Pull request description: It is unclear what benefit this option has, given that: * `gprof` requires re-compilation (`perf` and other tools can instead be used on existing executables) * `gprof` requires hardening to be disabled * `gprof` doesn't work with `clang` * `perf` is documented in the dev-notes, and test notes, and embedded into the functional test framework; `gprof` isn't * Anyone really wanting to use it could pass the required flags to `./configure` * I couldn't find any mention of the use of `gprof` in the discussions in this repo, apart from the initial pull request adding it (cfaac2a) * Keeping it means that it needs to be maintained and ported to CMake Fix all issues by removing it. ACKs for top commit: TheCharlatan: ACK fa780e1 hebasto: ACK fa780e1, I have reviewed the code and it looks OK. willcl-ark: crACK fa780e1 Tree-SHA512: 0a9ff363ac2bec8b743878a4e3147f18bc16823d00c5007568432c36320bd0199b13b6d0ce828a9a83c2cc434c058afaa64eb2eccfbd93ed85b81ce10c41760c
…nsaction` ab98e6f test: add coverage for errors for `combinerawtransaction` RPC (brunoerg) Pull request description: This PR adds test coverage for the following errors for the `combinerawtransaction` RPC: * Tx decode failed * Missing transactions * Input not found or already spent For reference: https://maflcko.github.io/b-c-cov/total.coverage/src/rpc/rawtransaction.cpp.gcov.html ACKs for top commit: maflcko: lgtm ACK ab98e6f tdb3: ACK ab98e6f Tree-SHA512: 8a133c25dad2e1b236e0278a88796f60f763e3fd6fbbc080f926bb23f9dcc55599aa242d6e0c4ec15a179d9ded10a1f17ee5b6063719107ea84e6099f10416b2
…ch32 encoding 07f6417 Reduce memory copying operations in bech32 encode (Lőrinc) d5ece3c Reserve hrp memory in Decode and LocateErrors (Lőrinc) Pull request description: Started optimizing the base conversions in [TryParseHex](bitcoin#29458), [Base58](bitcoin#29473) and [IsSpace](bitcoin#29602) - this is the next step. Part of this change was already merged in bitcoin#30047, which made decoding `~26%` faster. Here I've reduced the memory reallocations and copying operations in bech32 encode, making it `~15%` faster. > make && ./src/bench/bench_bitcoin --filter='Bech32Encode' --min-time=1000 Before: ``` | ns/byte | byte/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 19.97 | 50,074,562.72 | 0.1% | 1.06 | `Bech32Encode` ``` After: ``` | ns/byte | byte/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 17.33 | 57,687,668.20 | 0.1% | 1.10 | `Bech32Encode` ``` ACKs for top commit: josibake: ACK bitcoin@07f6417 sipa: utACK 07f6417 achow101: ACK 07f6417 Tree-SHA512: 511885217d044ad7ef2bdf9203b8e0b94eec8b279bc193bb7e63e29ab868df6d21e9e4c7a24390358e1f9c131447ee42039df72edcf1e2b11e1856eb2b3e10dd
7f3f6c6 refactor: replace hardcoded numbers (Lőrinc) 5676aec refactor: Model the bech32 charlimit as an Enum (josibake) Pull request description: Broken out from bitcoin#28122 --- Bech32(m) was defined with a 90 character limit so that certain guarantees for error detection could be made for segwit addresses (see https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#checksum-design). However, there is nothing about the encoding scheme itself that requires a limit of 90 and in practice bech32(m) is being used without the 90 char limit (e.g. lightning invoices, silent payments). Further, increasing the character limit doesn't do away with error detection, it simply changes the guarantee. The primary motivation for this change is for being able to parse BIP352 v0 silent payment addresses (see bitcoin@622c7a9), which require up to 118 characters. In addition to BIP352, modeling the character limit as an enum allows us to easily support new address types that use bech32m and specify their own character limit. ACKs for top commit: paplorinc: re-ACK 7f3f6c6 achow101: ACK 7f3f6c6 theuni: utACK 7f3f6c6 Tree-SHA512: 9c793d657448c1f795093b9f7d4d6dfa431598f48d54e1c899a69fb2f43aeb68b40ca2ff08864eefeeb6627d4171877234b5df0056ff2a2b84415bc3558bd280
fd6a7d3 test: use sleepy wait-for-log in reindex readonly (Matthew Zipkin) Pull request description: Also rename the busy wait-for-log method to prevent recurrence. See bitcoin#27039 (comment) ACKs for top commit: maflcko: utACK fd6a7d3 achow101: ACK fd6a7d3 tdb3: ACK for fd6a7d3 rkrux: ACK [fd6a7d3](bitcoin@fd6a7d3) Tree-SHA512: 7ff0574833df1ec843159b35ee88b8bb345a513ac13ed0b72abd1bf330c454a3f9df4d927871b9e3d37bfcc07542b06ef63acef8e822cd18499adae8cbb0cda8
…ndex e639364 validation: add missing insert to m_dirty_blockindex (Martin Zumsande) Pull request description: When the status of a block index is changed, we must add it to `m_dirty_blockindex` or the change might not get persisted to disk. This is missing from one spot in `FindMostWorkChain()`, where `BLOCK_FAILED_CHILD` is set. Since we have [code](https://github.com/bitcoin/bitcoin/blob/f0758d8a6696657269d9c057e7aa079ffa9e1c16/src/node/blockstorage.cpp#L284-L287) that later sets missing `BLOCK_FAILED_CHILD` during the next startup, I don't think that this can lead to bad block indexes in practice, but I still think it's worth fixing. ACKs for top commit: TheCharlatan: ACK e639364 stickies-v: ACK e639364 Tree-SHA512: a97af9c173e31b90b677a1f95de822e08078d78013de5fa5fe4c3bec06f45d6e1823b7694cdacb887d031329e4b4afc6a2003916e0ae131279dee71f43e1f478
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
WalkthroughThis PR consolidates three independent changes: (1) removes gprof profiling support from the build system and changes hardening default to enabled, (2) refactors the Bech32 checksum implementation with a new optional Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/bech32.cppsrc/bech32.cpp:5:10: error: 'bech32.h' file not found with include; use "quotes" instead ... [truncated 1293 characters] ... lugins/clang/install/lib/clang/18/include" src/validation.cppsrc/validation.cpp:7:10: error: 'validation.h' file not found with include; use "quotes" instead ... [truncated 2200 characters] ... ans.CTrans_funct.exec_with_node_creation in file "src/clang/cTrans.ml" (inlined), line 104, characters 20-38 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 |
|
✅ Review complete (commit 13d8299) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Six clean Bitcoin Core backports. The final tree at HEAD compiles and is semantically equivalent to upstream for the touched regions; the only in-scope concern is that bitcoin#29607 and bitcoin#30047 were cherry-picked in reverse upstream order, leaving one intermediate merge commit that references CHECKSUM_SIZE before it is defined and breaks bisect.
🟡 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 `src/bech32.cpp`:
- [SUGGESTION] src/bech32.cpp:189: Cherry-pick order of bitcoin#29607 and bitcoin#30047 is reversed — breaks git bisect
Upstream merged #30047 (charlimit enum / CHECKSUM_SIZE constexpr) before #29607 (encode-path memcopy reduction), and #29607 explicitly builds on #30047. This PR applies them in the opposite order: 804d22a487 (Merge #29607) lands before 621289fa9a (Merge #30047). At 804d22a487, src/bech32.cpp:189 reads `ret.reserve(hrp.size() + 1 + values.size() + CHECKSUM_SIZE);` but `constexpr size_t CHECKSUM_SIZE = 6;` is not introduced until the next merge. Verified: `git show 804d22a487:src/bech32.cpp` references CHECKSUM_SIZE, while neither 804d22a487~1 nor 804d22a487 itself defines it. That intermediate commit will not compile, which breaks `git bisect` across this range. The final HEAD (13d8299381) is correct because #30047 is applied immediately after, so this is non-blocking — but the merges should be reordered to match upstream and preserve bisectability.
| @@ -184,18 +188,18 @@ std::string Encode(Encoding encoding, const std::string& hrp, const data& values | |||
| // to return a lowercase Bech32/Bech32m string, but if given an uppercase HRP, the | |||
| // result will always be invalid. | |||
There was a problem hiding this comment.
🟡 Suggestion: Cherry-pick order of bitcoin#29607 and bitcoin#30047 is reversed — breaks git bisect
Upstream merged bitcoin#30047 (charlimit enum / CHECKSUM_SIZE constexpr) before bitcoin#29607 (encode-path memcopy reduction), and bitcoin#29607 explicitly builds on bitcoin#30047. This PR applies them in the opposite order: 804d22a (Merge bitcoin#29607) lands before 621289f (Merge bitcoin#30047). At 804d22a, src/bech32.cpp:189 reads ret.reserve(hrp.size() + 1 + values.size() + CHECKSUM_SIZE); but constexpr size_t CHECKSUM_SIZE = 6; is not introduced until the next merge. Verified: git show 804d22a487:src/bech32.cpp references CHECKSUM_SIZE, while neither 804d22a~1 nor 804d22a itself defines it. That intermediate commit will not compile, which breaks git bisect across this range. The final HEAD (13d8299) is correct because bitcoin#30047 is applied immediately after, so this is non-blocking — but the merges should be reordered to match upstream and preserve bisectability.
source: ['claude', 'codex']
4 participants
No description provided.