(+)#34132 l0rinc coins, dbwrapper: remove error catcher, make point-read failures fatal- Prevent database read failures and corruption from masquerading as missing coins
- Safely shut down the node instead of running with corrupt chainstate or rejecting valid transactions
- Simplify the coin view stack by eliminating the dedicated error catcher layer
| P2 · bug fix- CDBWrapper previously treated deserialization failures as missing entries, potentially masking disk or memory corruption across any subsystem using CDBWrapper. Standardizing failure-to-abort handling at the database wrapper level prevents silent data corruption bugs.
| Ready- Ready to review
- Clean state and CI passes after recent rebase
| 1 (+1) | Mild- General agreement on treating database read errors as fatal and simplifying the view stack
- Approval of code and test coverage (optout21)
- Concept approval for removing indirection (andrewtoth, sedited)
- Open concern whether corrupt optional indexes should crash the whole node (ajtowns)
| +292/-381 |
(+)#35301 Eunovo Silent Payments: Implement bip352 (take 2)- Implement BIP 352 silent payments logic to improve receiver privacy without address reuse
- Establish shared protocol primitives needed before the wallet can send or receive silent payments
| P2 · new feature- P2 because it supplies the foundational protocol primitives for silent payments in Bitcoin Core
- High leverage work that unblocks subsequent wallet sending, receiving, and descriptor tracking PRs
| Ready- Ready for review
- All previous feedback is addressed and tests pass with scope narrowed to standalone protocol logic
| 0 | Strong- Strong consensus on architecture and scope from multiple domain experts
- Concept approval from multiple contributors (w0xlt, rkrux, josibake)
- Extensive line-by-line review across multiple revisions (theStack)
- Agreed to narrow scope to standalone protocol primitives instead of touching wallet types (rustaceanrob, josibake)
| 828 + 6,118 tests- 828 lines added or modified outside tests
- 6,118 lines added or modified in tests
- 0 lines removed in total
- 10 files, 4 commits
|
(+)#34083 theuni Add initial vectorized chacha20 implementation for 2-3x speedup- Speed up ChaCha20 encryption to lower network thread CPU overhead under BIP324 transport
- Shorten compute-bound node synchronization phases by processing cipher blocks concurrently
| P2 · speedup- P2 because it yields a 2x to 3x speedup on a core crypto primitive without handwritten assembly
- ChaCha20 takes noticeable network CPU time in BIP324 and faster processing improves sync throughput
| Waiting on author- Author needs to address requested changes regarding platform regressions and test coverage
| 0 | Blocked- Broad support for ChaCha20 optimization gains
- Concept approval without stated reasons (jonatack)
- Verified performance improvements in local benchmarks (ajtowns)
- Unaddressed objection: benchmark regressions on low-power devices and missing tests (l0rinc)
| +434/-5 |
(+)#25665 ryanofsky refactor: Add util::Result failure types and ability to merge result values- Allow functions to return structured failure types alongside multiple warnings and error messages
- Eliminate ad-hoc out-parameters and tuple returns in multi-step operations like chainstate loading
| P2 · cleanup- P3 because it improves common error-handling primitives and lowers result memory use
- Unblocks cleaner error bubbling in wallet and validation code but is deferrable
| Ready- Ready for review, clean, and passing CI
- Very long comment thread makes navigation difficult
| 0 (+8) | Mild- Broad support across revisions from maintainers (laanwj, achow101, sedited)
- Debate over whether error payloads belong in util::Expected instead (hodlinator, maflcko)
- Suggestions to close and reopen cleanly due to thread length (maflcko, polespinasa, sedited)
| 532 + 200 tests- 532 lines added or modified outside tests
- 200 lines added or modified in tests
- 169 lines removed in total
- 13 files, 7 commits
|
(+)#35744 l0rinc coins: prevent DB resize from invalidating cursors- Prevent node crashes during concurrent UTXO scans and AssumeUTXO cache rebalancing
- Ensure UTXO database iterators remain valid when resizing the cache
| P2 · new feature- P2 because it adds shared mutex primitives with debug lockorder tracking to the codebase
- Enables non-LIFO lock releases without corrupting debug lock tracking state
| Ready- Ready for review
- Cleanly addresses reviewer feedback with passing CI and unit tests
| 0 | Strong- Strong collaboration shaping the shared mutex and sync framework design (andrewtoth)
- Suggested integrating shared locks into lockorder tracking with non-LIFO releases (andrewtoth)
- Concept approval without stated reasons (sedited)
| 89 + 105 tests- 89 lines added or modified outside tests
- 105 lines added or modified in tests
- 49 lines removed in total
- 9 files, 8 commits
|
(+)#31260 ryanofsky scripted-diff: Type-safe settings retrieval- Prevent recurring bugs and silent errors from misspelled option names, invalid types, or mismatched defaults
- Ensure configuration and command-line settings are checked at compile time across the codebase
| P2 · cleanup- P2 because it enforces compile-time safety across node configuration retrieval and registration
- Prevents a long-standing source of subtle bugs from misspelled argument names and mismatched defaults
- Unblocks planned runtime range and type checking across the configuration manager
| Needs rebase- Review is blocked by merge conflicts with master until the author rebases
| 0 | Mild- Strong concept support for compile-time guardrails on configuration settings
- Much better to let the compiler enforce settings safety (l0rinc)
- Praise for the safety improvements with testing and script refinements (hodlinator, jeanpablojp)
- Discussion remains open on handling edge cases and possibly splitting the repository-wide diff
| 2,831 + 882 tests- 2,831 lines added or modified outside tests
- 882 lines added or modified in tests
- 1,113 lines removed in total
- 100 files, 11 commits
|
(+)#27409 ryanofsky Make GUI and CLI tools use the same datadir- Ensure CLI tools automatically use the custom data directory selected in the GUI
- Prevent tools from downloading blocks to the wrong location or missing user wallets
| P3 · user request- P3 because it fixes a configuration trap where CLI tools diverge from GUI settings
- Stops bitcoind and CLI tools from defaulting to the wrong datadir and missing wallets
| Author reworking- Author stated it is not ready while working on complex Qt tests
- PR is currently marked draft and needs a rebase
| 0- No review verdicts recorded
| Mild- Positive concept feedback on resolving long-standing datadir divergence
- Welcomed as closing issue 8106 (pinheadmz)
- Clarified that custom datadir config files are still loaded properly (luke-jr)
| 455 + 10 tests- 455 lines added or modified outside tests
- 10 lines added or modified in tests
- 103 lines removed in total
- 13 files, 3 commits
|
(+)#35831 pablomartin4btc argsman, cli: Allow options after non-option arguments (GNU-style)- Allow CLI options to be placed after commands without being silently ignored or misparsed
- Prevents confusing errors or targeting the wrong wallet when passing options after an RPC command
| P3 · new feature- P3 because it fixes parser behavior where trailing options were silently swallowed as values
- Unrecognized trailing options are now properly caught and validated across consumers
| Ready- Ready to review because the author addressed previous feedback on edge cases and CI passes
| 1 | Strong- Strong support across the approach with edge cases tested and addressed
- Full support after suggesting edge-case fixes that the author incorporated (w0xlt)
- Verified tool consistency after concerns about bitcoin-tx behavior were resolved (vicjuma)
| 103 + 182 tests- 103 lines added or modified outside tests
- 182 lines added or modified in tests
- 38 lines removed in total
- 7 files, 3 commits
|
(+)#35422 nervana21 musig: Require generated secnonce for partial sig- Prevent crashes and undefined behavior when partial signing with uninitialized MuSig2 nonces
- Ensure nonce validity checks accurately reflect whether generation actually succeeded
| P3 · crash fix- P3 because it prevents crashes from uninitialized nonces in MuSig2 signing helpers
- P3 because it addresses an edge case found via fuzzing rather than standard usage
- P3 because it hardens cryptographic wrapper invariants against misuse
- Ranking pass: Moved up: uninitialized-nonce UB in a libsecp256k1 wrapper is a primitive-safety issue, which this category ranks above configuration and logging polish.
| Ready- Ready for review
- Reviewer feedback has been addressed and no blockers remain
| 0- No review verdicts recorded
| Neutral- Technical clarifications on libsecp nonce handling addressed (real-or-random)
- Suggested cleanup to defer nonce allocation was incorporated (achow101)
- No formal ACKs posted yet
| 20 + 30 tests- 20 lines added or modified outside tests
- 30 lines added or modified in tests
- 8 lines removed in total
- 4 files, 1 commits
|
(+)#36188 furszy crypto: plug hardware optimized SHA256 into libsecp256k1's context- Speed up signing operations by using hardware-accelerated SHA256 in secp256k1
- Unify and clean up the lifecycle management of cryptographic contexts
| P3 · speedup- P3 because signing is faster on CPUs with SHA extensions but verification speed is largely unaffected
- Improves context lifecycle management across the codebase without fixing any critical safety issues
- Ranking pass: Measured 13%/7% signing speedup in a core crypto path plus context lifecycle cleanup; top of P3 just below the 2-3x chacha20 P2.
| Ready- Ready to review
- Applies cleanly with passing CI and no open questions or objections
| 0 | Positive- Broad concept support across contributors and maintainers with full code review pending
- Enthusiastic concept approval (sedited)
- Concept approval without stated reasons (w0xlt, fjahr, ismaelsadeeq, theStack, hebasto)
- Notified upstream maintainer for awareness (fanquake)
| 222 + 230 tests- 222 lines added or modified outside tests
- 230 lines added or modified in tests
- 151 lines removed in total
- 43 files, 11 commits
|
(+)#35763 kevkevinpal util: write JSON atomically in WriteJson- Prevent corrupted or truncated node configuration and banlist files if the process is interrupted
- Consolidate safe atomic file replacement into shared settings write utilities
| P3 · crash safety- P3 because it improves crash safety and persistence robustness for node configuration and banlist files
- Prevents file corruption or truncation if the node terminates during serialization
- Ranking pass: Moved up: crash-safe atomic writes for settings.json/banlist.json is persistence robustness, ranked with the other file-handling correctness fixes.
| Ready- Ready for review, with all reviewer questions and suggestions addressed in the latest update
| 0 (+1) | Strong- Broad agreement on making atomic replacement the default in shared write utilities
- Supported making atomic writes standard and verified test coverage (Herb-ops, winterrdog)
- Concerns regarding naming symmetry and temporary file accumulation were resolved (winterrdog)
| 28 + 45 tests- 28 lines added or modified outside tests
- 45 lines added or modified in tests
- 24 lines removed in total
- 7 files, 1 commits
|
(+)#16545 ryanofsky refactor: Implement missing error checking for ArgsManager flags- Prevent startup argument errors and confusing configuration bugs caused by untyped strings
- Establish validation logic to detect malformed options and incompatible configuration flags
| P3 · cleanup- P3 because direct user benefit is deferred until existing options adopt the typed flags
- P3 because it provides cleanup and leverage for follow-up fixes to configuration quirks
- Ranking pass: Placed ahead of 17580/17493/17783 because it is their base; reviewing it first is what unblocks the stack.
| Review #31260 first- Review #31260 first
- Author suggests prioritizing #31260 because its C++ type declarations may alter or clarify this PR
| 0 (+1) -1 | Positive- General support for enforcing stronger, consistent argument error checking
- Concept approval for incremental strict typing of startup options (laanwj, hodlinator)
- Approach approval with suggested refactorings incorporated (l0rinc)
- Objection on flag complexity addressed by removing multi-flag combinations (maflcko)
- Dormant concern regarding flag ergonomics and default-reset behavior (ajtowns)
| 309 + 542 tests- 309 lines added or modified outside tests
- 542 lines added or modified in tests
- 90 lines removed in total
- 9 files, 6 commits
|
(+)#17580 ryanofsky refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags- Prevent silent configuration bugs when reading multi-value options across the codebase
- Ensure developers retrieve all values consistently instead of accidentally taking only the first or last
| P3 · cleanup- P3 because it improves shared configuration safety across the codebase by avoiding subtle option bugs
- Unblocks follow-up work forbidding duplicate conflicting configuration options
| Review #16545 first- Review #16545 first
- Code is up to date and passing CI on top of open base PR #16545
| 0 | Positive- Concept approval without stated reasons (hebasto, promag)
- Concept approval with technical feedback on validation checks addressed by author (ajtowns)
- Marked draft while waiting on the underlying PR (fanquake)
| 377 + 767 tests- 377 lines added or modified outside tests
- 767 lines added or modified in tests
- 197 lines removed in total
- 15 files, 14 commits
|
(+)#17493 ryanofsky util: Forbid ambiguous multiple assignments in config file- Fail startup with an explicit error when single-value settings are assigned multiple times in a config file
- Prevent node operators from mistakenly assuming a discarded duplicate setting is active
| P3 · user request- P3 because it eliminates silent misconfigurations from duplicate single-value options
- Protects node operators from invalid assumptions and unblocks cleanup in #17581
- Ranking pass: Moved below its bases 16545/17580, which must be reviewed first for this to be actionable.
| Review #16545 first- Review #16545 first
- PR is stacked on #16545 and #17580 with two distinct commits of its own
| 0 -1 | Positive- General support for catching duplicate configuration errors on startup
- Concept approval while weighing potential impact on existing configs (JeremyRubin)
- Concern over restart failures on upgrade resolved by using release notes (ajtowns)
| 388 + 808 tests- 388 lines added or modified outside tests
- 808 lines added or modified in tests
- 203 lines removed in total
- 16 files, 17 commits
|
(+)#17783 ryanofsky common: Disallow calling IsArgSet() on ALLOW_LIST options- Prevent negated or empty list options from mistakenly appearing as enabled
- Avoid subtle configuration and security bugs caused by misinterpreting command line options
| P3 · bug fix- P3 because it eliminates an internal hazard where empty list options falsely appear configured
- Systematically protects against recurring configuration and security mistakes across the codebase
| Review #16545 first | 0 | Positive- Positive sentiment with minimal engagement while waiting on base pull requests
- Concept approval without stated reasons (promag)
| 391 + 793 tests- 391 lines added or modified outside tests
- 793 lines added or modified in tests
- 207 lines removed in total
- 15 files, 18 commits
|
(+)#35387 ryanofsky logging: make trace logging easily usable- Make trace logging usable without requiring paired debug flags
- Allow node operators to inspect and adjust log levels per category at runtime without restarting
| P3 · user request- P3 because it fixes unintuitive configuration where loglevel had no effect without debug
- Makes trace logging discoverable and straightforward to configure
| Author silent- Author silent for over 60 days, though CI passes and the branch has no conflicts
| 0- No review verdicts recorded
| Positive- Praised the new interface design as elegant and intuitive (stickies-v)
- Nonblocking suggestion to deprecate the older logging RPC to avoid maintaining both (stickies-v)
| 188 + 102 tests- 188 lines added or modified outside tests
- 102 lines added or modified in tests
- 41 lines removed in total
- 11 files, 2 commits
|
(+)#34038 ajtowns logging: replace -loglevel with -trace, expose trace logging via RPC- Make trace logging straightforward to configure at startup alongside debug logs
- Allow node operators to dynamically inspect and toggle trace logging at runtime via RPC
| P3 · new feature- P3 because it eliminates a confusing pitfall where trace logging was silently ignored
- Improves developer ergonomics and lays groundwork for wider trace logging adoption
- Ranking pass: Placed immediately after the competing 35387: same problem, and the pending design decision is what blocks both.
| Ready- Ready for review, with clean code and passing tests
- An open design dispute over command-line flag syntax invites feedback
| 2 -1 | Disputed- Split on whether to use parallel flags or a unified key-value option
- Support because the parallel startup flags are much easier to use (stickies-v, sedited)
- Approach objection: interacting flags are confusing compared to a single option (ryanofsky)
| +266/-132 |
(+)#35322 ryanofsky logging: streamline Logger state and drop redundant methods- Avoid mutex acquisition and overhead during routine logging checks across the node
- Simplify internal logger state management and remove redundant category methods
| P3 · cleanup- P3 because it eliminates lock acquisition and map lookups on every category log check
- Improves performance across all logging callers while removing redundant state and methods
- Ranking pass: Moved up: removes mutex acquisition from every category check and simplifies Logger state, with no unresolved approach dispute, unlike the -trace variants.
| Ready- Ready for review
- Cleanly rebased on master with passing CI and previous review feedback addressed
| 0- No review verdicts recorded
| Neutral- Requested splitting commits and preserving flag behavior when debug mode is off (l0rinc)
- Requested explicit tests for category enablement through RPC and kernel interfaces (l0rinc)
- Author addressed all review comments and added tests, but follow-up review is pending
| 105 + 100 tests- 105 lines added or modified outside tests
- 100 lines added or modified in tests
- 196 lines removed in total
- 14 files, 14 commits
|
(+)#35936 l0rinc net: reject oversized locators before allocating- Disconnect peers that send oversized block locator counts before reading their payloads
- Avoid unneeded memory allocations when parsing malformed getblocks and getheaders messages
| P3 · cleanup- P3 because it lets serialization utilities distinguish size limit violations from generic parse corruptions
- It allows callers across the codebase to cleanly catch and handle vector limit breaches
- Ranking pass: Moved up: hardens serialization limits against malformed p2p payloads, which outranks diagnostics and type-safety cleanups here.
| Ready- Ready for review as branch is clean and all reviewer feedback has been addressed
| 0 (+2) | Positive- Tested and approved after proposing a fix for counts above MAX_SIZE (jeanpablojp)
- Approved the approach and implementation (achow101)
- Endorsed the exception-based design over duplicating stream parsing loops (ajtowns)
- Questioned parser consistency with headers without blocking (mzumsande, sedited)
| 36 + 26 tests- 36 lines added or modified outside tests
- 26 lines added or modified in tests
- 20 lines removed in total
- 4 files, 4 commits
|
(+)#36234 JeremyRubin util: avoid redundant rolling Bloom insertions- Prevent repeated insertions of the same item from prematurely expiring older entries in rolling filters
- Ensures filters retain tracked items up to their configured capacity even under duplicate inputs
| P3 · bug fix- P3 because rolling Bloom filters are shared across networking to track seen items without early eviction
- Prevents duplicate items from purging older data, though no caller vulnerability or benchmark was shown
- Ranking pass: Moved up: correctness of a shared filter primitive relied on by networking ranks above internal ergonomics work.
| Ready- Ready for review
- Small patch with passing CI and dedicated unit tests
| 0- No review verdicts recorded
| Crickets- No concept or approach reviews yet
- Asked whether this fixes a known caller problem or is just a general correctness improvement (instagibbs)
| 22 + 52 tests- 22 lines added or modified outside tests
- 52 lines added or modified in tests
- 3 lines removed in total
- 3 files, 1 commits
|
(+)#36011 ryanofsky btcsignals: destroy callbacks on disconnect- Destroy disconnected signal callbacks immediately instead of deferring cleanup
- Prevent shutdown hangs and deadlocks when callbacks hold resources with release side effects
| P3 · bug fix- P3 because it fixes shutdown deadlocks and unpredictable resource retention in utility signals
- Essential for multiprocess GUI support, though deferrable for the current release
| Ready- Ready to review now with passing CI and new characterization tests
| 0- No review verdicts recorded
| Crickets- General agreement on the need for the change without objections
- Acknowledged as necessary to fix multiprocess GUI shutdown issues (maflcko)
- Agreed that merging is not urgent for the upcoming release (maflcko, ryanofsky)
| 69 + 46 tests- 69 lines added or modified outside tests
- 46 lines added or modified in tests
- 9 lines removed in total
- 2 files, 3 commits
|
(+)#35281 ferminquant univalue: respect token end pointer- Prevent out-of-bounds memory reads when parsing JSON inputs that lack null terminators
- Protect node operators and callers from memory safety bugs during bounded text parsing
| P3 · bug fix- P3 because it fixes two out-of-bounds memory safety bugs in shared utility parsing code
- Restricts benefits to callers passing non-null-terminated string views
| Needs rebase- Needs rebase due to merge conflicts with the base branch
- Author has been inactive for over a month
| 0 (+1) | Mild- General agreement on fixing the out-of-bounds memory reads in JSON parsing
- Concept approval noting it fixes two distinct out-of-bounds reads (sedited)
- Verified by testing bounded input test cases (carloantinarella)
- Suggested minor test cleanup around buffer handling (maflcko)
| 31 + 3 tests- 31 lines added or modified outside tests
- 3 lines added or modified in tests
- 31 lines removed in total
- 10 files, 1 commits
|
(+)#36049 l0rinc streams: avoid termination on buffered write failure- Prevent abrupt node crashes when encountering storage errors during block writing
- Allow standard storage error handling and graceful shutdown when disks run out of space
| P3 · bug fix- P3 because it fixes an exception safety bug in buffered streams
- Prevents a destructor from triggering an immediate crash during exception unwinding
| Ready- Ready for review as the patch is small and CI is passing
| 0 | Strong- General agreement that the bug exists and should be addressed
- Concept approval after independently hitting the identical bug (w0xlt)
- Inquired whether terminating was acceptable for storage errors, which author resolved (andrewtoth)
| 6 + 14 tests- 6 lines added or modified outside tests
- 14 lines added or modified in tests
- 3 lines removed in total
- 3 files, 3 commits
|
(+)#29700 ryanofsky kernel, refactor: return error status on all fatal errors- Return fatal error details programmatically instead of aborting the node process during validation
- Let libbitcoinkernel users handle critical failures directly without relying on notification callbacks
| P3 · cleanup- P3 because expanding result framework capabilities is mostly auxiliary to the kernel work
- General utility improvements provide limited independent benefit on their own
| Review #25665 first | 0- No review verdicts recorded
| Blocked- Blocked by an unaddressed approach objection
- Unaddressed objection: Result-style error handling is unidiomatic and harms performance (purpleKarrot)
- Nonblocking objection: nesting interrupt results inside error results invites unhandled stops (maflcko)
- Approval without detailed technical analysis (Graysonbarton)
| 1,314 + 440 tests- 1,314 lines added or modified outside tests
- 440 lines added or modified in tests
- 675 lines removed in total
- 55 files, 22 commits
|
(+)#19461 ryanofsky multiprocess: Add bitcoin-gui -ipcconnect option- Connect a GUI directly to an already running node process over IPC
- Allow users to start and close the graphical interface without shutting down the node
| P3 · cleanup- Improves `btcsignals` by making `disconnect()` eagerly destroy callbacks and their associated state, fixing a problem where node notification callbacks owning IPC proxy objects kept processes from exiting cleanly.
| Review #19460 first | 0 | Mild- Broad agreement on the feature and interface design
- Concept approval noting consistency with rpcconnect conventions (laanwj)
- Concept approval without stated reasons (meshcollider)
- Verified by testing remote GUI connections over an SSH socket (Sjors)
- Identified node crash on wallet flag mismatch, tracked as a follow-up (jimhashhq)
| 3,912 + 135 tests- 3,912 lines added or modified outside tests
- 135 lines added or modified in tests
- 567 lines removed in total
- 142 files, 27 commits
|
(+)#10102 ryanofsky Multiprocess bitcoin- Isolate the node, wallet, and GUI into separate processes so crashes or exploits do not compromise the node
- Allow components to run on independent lifecycles for improved stability and security
| P3 · bug fix- P3 because it prevents callback state from being retained indefinitely after disconnect
- It eliminates test hangs and delays when shutting down processes
| Review #29409 first | 0 (+1) -1 | Strong- Strong long-standing concept consensus across multiple maintainers
- Concept approval emphasizing the value of process isolation (laanwj)
- Concept approval with testing (Sjors)
- Historical concept approval (dcousens, promag, jamesob, hebasto)
| 3,842 + 135 tests- 3,842 lines added or modified outside tests
- 135 lines added or modified in tests
- 539 lines removed in total
- 136 files, 23 commits
|
(+)#36190 Bortlesboat test: Check bitcoin wrapper child exit status on windows- Verify that the Windows bitcoin wrapper blocks until its child finishes and correctly forwards exit codes
- Ensure scripts and callers invoking bitcoind via the wrapper get reliable exit codes and error reports
| P3 · test coverage- P3 because it pins process spawning and error propagation under Windows CRT quirks
- Prevents regressions in return code handling and errno mapping
| Review #36105 first- Review #36105 first
- Code is ready, but it depends on the runtime wrapper fixes in base PR #36105
| 1 | Strong- Strong support with cross-platform testing
- Verified on native Windows runners with UCRT and MSVCRT (cyb3ralbert)
- Supported splitting this test out into a dedicated PR (ryanofsky)
| 27 + 52 tests- 27 lines added or modified outside tests
- 52 lines added or modified in tests
- 19 lines removed in total
- 6 files, 6 commits
|
(+)#30342 ryanofsky kernel, logging: Pass Logger instances to kernel objects- Allow kernel library consumers to route log output per instance rather than through a global logger
- Help multi-instance applications segregate and control their own log streams
| P3 · cleanup- P3 because contextual log macros add conceptual complexity to logging utilities ahead of direct need
- Refactoring log utilities remains optional until downstream subsystems require instanced routing
| Needs rebase- Wait for rebase due to substantial merge conflicts across dozens of files
- Review #34778 first
| 0 -1 | Disputed- Disputed whether instanced logging justifies the interface verbosity and maintenance cost
- Concept approval without stated reasons (sedited)
- Approach objection: review cost and verbosity outweigh benefits without consumer demand (stickies-v)
- Prefers keeping logging callback explicitly global via alternative PR #34775 (stickies-v)
| 723 + 376 tests- 723 lines added or modified outside tests
- 376 lines added or modified in tests
- 591 lines removed in total
- 69 files, 18 commits
|
(+)#35833 l0rinc log: prevent user input from injecting fake log lines- Prevent untrusted input from forging fake lines or misleading banners in debug.log
- Protect node operators relying on log integrity from RPC or external daemon injection
| P3 · security- P3 because it fixes an established log injection vulnerability from untrusted inputs
- Improves diagnostic integrity for node operators without fixing an active crash or consensus flaw
| Ready- Ready to review
- CI is passing and author addressed requested architectural cleanups
| 0 (+3) | Strong- Strong consensus on central newline escaping after resolving earlier approach and scope objections
- Supports central escaping over per-site sanitizing to cover all log sources (ryanofsky, davidgumberg)
- Verified injection paths are blocked by manual testing (polespinasa)
- Concept approval without stated reasons (achow101)
| 31 + 44 tests- 31 lines added or modified outside tests
- 44 lines added or modified in tests
- 18 lines removed in total
- 9 files, 3 commits
|
(+)#26966 furszy index: initial sync speedup, parallelize process- Speed up initial construction of optional indexes by processing blocks in parallel across worker threads
- Dramatically cut the hours needed to build block filters and transaction indexes on multi-core systems
| P3 · new feature- P3 because the thread pool is an internal utility supporting other subsystems
- It is reasonably deferrable on its own without direct user-facing benefit
| Rebase after #33689- Review #33689 first
- Drafted by the author and needs rebase
| 0 (+1) -1 | Disputed- Strong support for parallelizing sync, but contested over diff complexity and txindex regressions
- Concept approval for the design and approach (ryanofsky)
- Verified sync time dropped from over 48 hours to 16 hours in testing (pinheadmz)
- Approach objection over diff complexity and performance regressions on txindex (l0rinc)
- Observed slight txindex slowdowns on SSD and questioned parallelizing it (andrewtoth)
| 569 + 462 tests- 569 lines added or modified outside tests
- 462 lines added or modified in tests
- 153 lines removed in total
- 14 files, 11 commits
|
(+)#35557 ryanofsky kernel, validation: Add btck_chainstate_manager_set_clock_time- Allow kernel users and multi-instance tests to mock time per instance without relying on global state
- Prevent kernel validation code from accidentally calling nondeterministic host system clocks
| P3 · cleanup- P3 because isolating nondeterministic clock definitions enforces link-time safety for the kernel
- Prevents the kernel library from inadvertently calling host system time
| Ready- Ready to review: branch builds cleanly, passes CI, and is actively maintained across rebases
| 0- No review verdicts recorded
| Strong- Positive sentiment with constructive review on typing, testing, and clock ergonomics
- Supported keeping the changes unified and requested scoped clock tests (sedited)
- Commended the test race fix and suggested using a fake clock (seduless)
- Questioned whether this is still necessary after global mock time merged (maflcko)
| 172 + 86 tests- 172 lines added or modified outside tests
- 86 lines added or modified in tests
- 99 lines removed in total
- 31 files, 6 commits
|
(+)#36105 ryanofsky bitcoin wrapper: Fix Windows exec so wrapper waits for child process- Make the Windows bitcoin wrapper wait for child processes to finish and propagate their exit codes
- Allow test frameworks and caller scripts on Windows to capture output and monitor child exit status
| P3 · platform fix- Improves shared utility infrastructure in src/util/exec.cpp by ensuring process launching functions on Windows behave consistently with expectations, waiting on child handles and mapping error categories properly.
| Ready- Ready to review
- All CI checks are passing and recent review comments have been addressed
| 0 | Mild- Strong support for fixing Windows child process waiting without objections to the approach
- Concept approval and runtime error handling review (hodlinator)
- Concept approval with notes on future UCRT transitions (hebasto)
- Collaborated on child exit code resolution logic (Bortlesboat)
| 27 + 11 tests- 27 lines added or modified outside tests
- 11 lines added or modified in tests
- 20 lines removed in total
- 6 files, 4 commits
|
(+)#35688 l0rinc crypto: accept empty HMAC keys- Allow cryptographic HMAC functions to accept empty keys without triggering undefined behavior
- Eliminates UBSan warnings and removes empty-input workarounds across fuzz targets
| P3 · bug fix- P3 because resolving undefined behavior on empty keys cleans up sanitizer runs
- Enables fuzz testing to exercise empty keys directly without special-casing
- Ranking pass: Moved up to sit with the other crypto/primitive UB fixes (36286, 35818) rather than among diagnostics changes.
| Author silent- Author has been silent for over 60 days
- Code is small, rebased, and CI is passing
| 1 | Positive- General support after author resolved all implementation and cleanup comments
- Suggested using standard copy and cleaning up commits (maflcko)
- Approved the updated commits (sedited)
| 4 + 5 tests- 4 lines added or modified outside tests
- 5 lines added or modified in tests
- 22 lines removed in total
- 5 files, 2 commits
|
(+)#36286 fjahr crypto: Fix MuHash3072 division by itself- Fix calculation bugs when dividing a MuHash3072 object by itself
- Ensures mathematical correctness in cryptographic utility operations
| P3 · bug fix- P3 because it fixes a latent calculation bug in a cryptographic primitive
- No production code currently performs self-division, leaving active nodes unaffected
- Ranking pass: Moved up to be adjacent to the equivalent primitive-correctness fixes; no production caller, so it sits at the bottom of that cluster's top edge.
| Ready- Ready to review
- The change is tiny, CI passes, and no changes are pending
| 0- No review verdicts recorded
| Crickets- No reviews or discussion yet
- PR was recently opened and has received no feedback so far
| 2 + 7 tests- 2 lines added or modified outside tests
- 7 lines added or modified in tests
- 1 lines removed in total
- 3 files, 1 commits
|
(+)#35818 l0rinc bloom: avoid undefined sizing calculations- Avoid division by zero and undefined floating-point conversions during bloom filter sizing
- Prevents crashes and undefined behavior when running unit tests and fuzz targets with edge-case inputs
| P3 · bug fix- P3 after comparing with the other PRs here (assessed alone as P4): Raised P4->P3 for consistency with 36286 and 35688: all three fix UB/aliasing in primitives with no current production caller and must share a band.
- P4 because the fix prevents undefined behavior and crashes during bloom filter sizing
- Limited to edge cases in unit tests and fuzz targets rather than production node paths
| Ready- Ready for review with passing CI and no blocking requests
| 1 | Positive- Full code review approval with no remaining objections (rustaceanrob)
- Agreed to defer moving test construction logic into a helper to a follow-up (rustaceanrob)
| 13 + 62 tests- 13 lines added or modified outside tests
- 62 lines added or modified in tests
- 4 lines removed in total
- 4 files, 4 commits
|
(+)#36210 pablomartin4btc streams: Include the OS error when AutoFile I/O fails- Include the underlying OS error message when file stream I/O operations fail
- Help users and callers diagnose exact failure causes like missing permissions or full disks
| P3 · diagnostics- P3 because it preserves errno to give meaningful failure details across the node
- Provides a durable diagnostic improvement to shared file utilities without addressing critical bugs
- Ranking pass: Moved up: durable diagnostic improvement to shared file streams that every area benefits from.
| Ready- Ready for review
- Small and self-contained with passing tests and no blockers
| 0- No review verdicts recorded
| Crickets- No reviews or concept feedback yet
| 12 + 98 tests- 12 lines added or modified outside tests
- 98 lines added or modified in tests
- 5 lines removed in total
- 3 files, 2 commits
|
(+)#35948 l0rinc init: correct first-run disk space estimate- Warn users with accurate required disk space on first startup
- Prevent misleading pruned node operators into thinking they need full chain storage
| P3 · bug fix- P3 because it fixes an inaccurate startup warning for pruned node operators
- Prevents confusion by showing actual prune targets instead of hundreds of gigabytes
- Ranking pass: Moved up: fixes a misleading user-facing startup warning, which this category ranks above internal cleanups.
| Ready- Ready to review with a small diff, passing CI, and addressed feedback
| 3 (+1) | Strong- Strong consensus across reviewers to fix the misleading startup warning text
- Verified expected warning behavior on pruned setups (murchandamus, jeanpablojp)
- Supported keeping internal units in GiB while fixing displayed strings (achow101, mercie-ux)
- Agreed that remaining GUI unit inconsistencies can be handled in a follow-up (hebasto)
| 5- 5 lines added or modified outside tests
- 0 lines added or modified in tests
- 4 lines removed in total
- 2 files, 1 commits
|
(+)#35195 l0rinc coins: cache UTXO outpoint hash codes- Speed up UTXO cache lookups during Initial Block Download and AssumeUTXO loading
- Trade a small amount of memory to avoid recomputing hashes on table operations
| P3 · speedup- P3 because benchmarks show clear speed improvements in utility hash map containers
- Trades a modest increase in node memory to avoid repeated hashing
| Needs rebase- Review is blocked by merge conflicts and author inactivity
| 0 | Strong- Concept approval because benchmarks show speedup justifies memory use (optout21)
- Open question whether caching should use __is_fast_hash instead (optout21)
| 4 + 17 tests- 4 lines added or modified outside tests
- 17 lines added or modified in tests
- 7 lines removed in total
- 2 files, 2 commits
|
(+)#34844 maflcko util: Add util::NotNull<SmartPtrType>- Enforce non-null smart pointer contracts at construction and function boundaries
- Avoid repetitive defensive assertion checks and clarify API expectations across the codebase
| P3 · new feature- P3 because it provides a reusable type invariant that simplifies null handling across the codebase
- Modest leverage improvement by replacing repetitive runtime assertions with clearer API contracts
| Ready- Ready for review with a settled implementation and passing tests
| 1 (+1) | Strong- Strong support after co-authoring the simplified implementation (stickies-v)
- Concept approval without detailed comments (sedited)
- Approved earlier iterations prior to the custom rewrite (l0rinc)
| 134 + 246 tests- 134 lines added or modified outside tests
- 246 lines added or modified in tests
- 65 lines removed in total
- 13 files, 5 commits
|
(+)#35511 hodlinator RFC: consensus: Make `CAmount` a class- Prevent accidental monetary bugs like invalid conversions or nonsensical math for developers
- Catch uninitialized coin values and invalid operations at compile time across the codebase
| P3 · type safety- P3 because it eliminates an entire class of monetary calculation bugs across all subsystems
- Provides broad architectural safety but does not address an active incident and is safely deferrable
- Ranking pass: Lowered relative to its score: broad type-safety win but no active defect and explicitly needs splitting, so deferrable behind concrete fixes.
| Ready- Ready for review, the branch builds cleanly and passes CI
| 0 | Disputed- Broad concept support for stronger monetary type safety
- Concept approval without stated reasons (sedited)
- Prefers incremental migration over taking the entire codebase transition at once (ryanofsky)
- Changes requested: monolithic diff is too large and needs invariant clarification (l0rinc)
| 574 + 912 tests- 574 lines added or modified outside tests
- 912 lines added or modified in tests
- 1,258 lines removed in total
- 184 files, 12 commits
|
(+)#26022 ryanofsky Add util::ResultPtr class- Make checking and accessing pointer results safer and less awkward for developers
- Prevents accidental null pointer dereferences when a function returns a null pointer without an error
| P3 · cleanup- P3 because it provides an ergonomic and safety improvement to shared result-handling utilities
- Reduces syntax awkwardness and null-check bugs but is internal convenience that can wait
- Ranking pass: Kept below its base 25665; ergonomic wrapper whose value depends on that PR landing first.
| Review #25665 first | 0- No review verdicts recorded
| Mild- No overall reviews yet
- Unaddressed question: possible inverted condition in wallet restore (l0rinc)
| 578 + 235 tests- 578 lines added or modified outside tests
- 235 lines added or modified in tests
- 199 lines removed in total
- 19 files, 11 commits
|
(+)#36087 maflcko util: Add and use AssertUnreachable- Provide a dedicated unreachable assertion to resolve assertion ambiguity and silence compiler warnings
- Prevent undefined behavior across the codebase from future std::unreachable usage
| P3 · cleanup- P3 because it improves code cleanliness and silences GCC return-type warnings
- Directly benefits developers by guarding against undefined behavior from std::unreachable
- Ranking pass: Moved up: eliminates a real UB hazard from future std::unreachable use across the tree, above cosmetic diagnostics work.
| Needs rebase- Needs rebase and CI is failing
- Author is reworking to fix consensus library decoupling before taking out of draft
| 0- No review verdicts recorded
| Mild- No reviewers have formally approved the concept yet
- Unaddressed objection: breaks standalone consensus library build (jeanpablojp)
- Author agreed to fix the consensus library dependency before readying the PR (maflcko)
| 368 + 422 tests- 368 lines added or modified outside tests
- 422 lines added or modified in tests
- 546 lines removed in total
- 312 files, 7 commits
|
(+)#36183 kevkevinpal span: diagnose dangling views from MakeByteSpan/MakeUCharSpan- Catch dangling views created from temporary objects at compile time
- Prevent silent use-after-free bugs when converting temporary containers to spans
| P3 · cleanup- P3 because it helps catch developer lifetime mistakes without fixing an active runtime failure
- Adding compile-time lifetime diagnostics to utilities has modest leverage and is safe to defer
- Ranking pass: Moved up: compile-time lifetime diagnostics for widely used span helpers, though the design needs rework.
| Waiting on author- Worth reviewing now to assess how to avoid false warnings on non-owning views
| 0 | Mild- Concept approval with reservations about false positive diagnostics (jeanpablojp)
- Unaddressed concern that lifetime annotations trigger false warnings on borrowed views (jeanpablojp)
| 8 + 6 tests- 8 lines added or modified outside tests
- 6 lines added or modified in tests
- 7 lines removed in total
- 4 files, 1 commits
|
(+)#34778 ryanofsky logging: rewrite macros to enforce restrictions at compile-time, improve efficiency and usability- Catch misuse of logging categories at compile time with clear error messages
- Make argument evaluation consistent and avoid formatting overhead when logging is disabled
| P3 · cleanup- P3 because benefits are developer-facing cleanups and minor logging efficiency gains.
- Clearer compile-time diagnostics help developers catch category misuses earlier.
- Ranking pass: Placed ahead of 29256/30342 as their base; the approach dispute here gates the whole logging-context chain.
| Author silent- Author has been silent for several months with unaddressed objections and a standing Concept NACK
| 0 -1 | Disputed- Significant pushback against the macro design with prolonged author inactivity
- Concept NACK against reintroducing a generic logging macro interface (ajtowns)
- Unaddressed objection: flexible macro risks bypassing log levels and harms greppability (maflcko)
- Questioned keeping the PR open given prolonged inactivity and the unresolved NACK (sedited)
| 159 + 116 tests- 159 lines added or modified outside tests
- 116 lines added or modified in tests
- 113 lines removed in total
- 6 files, 6 commits
|
(+)#29256 ryanofsky log, refactor: Allow log macros to accept context arguments- Allow logging macros to accept context arguments instead of requiring a global singleton
- Enable callers to prepend metadata like wallet names or route logs to isolated streams
- Help library consumers like libbitcoinkernel separate logging instances
| P3 · unblocks #30342- P3 because it lets macros attach context cleanly without separate macro implementations per subsystem
- Unblocks instanced kernel logging in #30342 and wallet logging refactoring in #30343
- Does not fix an active bug or performance issue in bitcoind
- Ranking pass: Moved below 34778, its base; the contextual-logging concept is also disputed, so its standalone value is deferred.
| Author silent 113d- Review #34778 first
- Author inactive for over 100 days
| 0 -2 | Disputed- Concept objection: instanced logging is unnecessary complexity and churn (ajtowns)
- Concept approval: useful for distinguishing concurrent outputs like multiple chainstates (sedited)
- Concept approval: supports simpler and more consistent logging APIs across the project (jonatack)
- Requested changes regarding output behavior and category filtering enforcement (hodlinator)
| 210 + 207 tests- 210 lines added or modified outside tests
- 207 lines added or modified in tests
- 114 lines removed in total
- 6 files, 10 commits
|
(+)#34729 ajtowns Reduce log noise- Prevent misleading high-severity errors and log spam during routine network and proxy connection failures
| P3 · cleanup- P3 because it provides a reusable rate-limited logging pattern across multiple modules
- It eliminates misleading error-level reporting across the codebase
- Ranking pass: Lowered: log-noise demotions are operator-facing polish with a reviewer reservation, below correctness work in this category.
| Ready | 0 (+4) | Positive w/ caveats- General support for reducing log noise and adding the helper (sedited, l0rinc)
- Nonblocking objection: demoting error levels makes diagnosing failures harder (ryanofsky)
| 61 + 24 tests- 61 lines added or modified outside tests
- 24 lines added or modified in tests
- 54 lines removed in total
- 13 files, 7 commits
|
(+)#35676 maflcko util: Abort in CheckDiskSpace/FlatFileSeq::Open on rare exceptions- Log descriptive error messages when disk space checks fail instead of crashing without diagnostics
- Ensures node operators can diagnose termination caused by disconnected or failing storage media
| P3 · stability- P3 because prevents silent crashes on filesystem errors but occurs only in rare environment conditions
- Node operators gain clearer diagnostics when storage fails, but regular operation is unaffected
- Ranking pass: Lowered: multiple reviewers dispute aborting inside low-level helpers, and 36049 addresses the same disk-error class more conservatively.
| Author silent 64 days- Stale: author silent for over 60 days while fundamental approach objections remain unaddressed
| 0 | Disputed- Disputed over whether low-level utility helpers should abort or propagate errors
- Unaddressed objection: low-level helpers should not make process termination decisions (josibake, optout21)
- Unaddressed objection: abrupt abort prevents clean flushes of wallet and chainstate data (furszy)
- Prefers catching exceptions at thread boundaries to trigger graceful shutdown (sedited)
| 24 + 131 tests- 24 lines added or modified outside tests
- 131 lines added or modified in tests
- 3 lines removed in total
- 6 files, 2 commits
|
(+)#24773 hebasto Enable HW-accelerated implementations of SHA256 for MSVC builds- Bring hardware-accelerated SHA256 hashing to MSVC builds on Windows
- Eliminate significant hashing performance penalties for native Windows developer builds
| P3 · speedup- P3 because hashing operations achieve five- to ten-fold speed improvements
- Impact is confined to native Windows development rather than end users
- Ranking pass: Lowered: large speedup but confined to MSVC developer builds, not release binaries, and blocked on whether MSVC stays supported.
| Waiting on author- Paused while waiting on author response regarding a project decision to close it
- Maintainers decided in a meeting to drop MSVC support in favor of clang-cl in #31507
| 0 (+1) | Blocked- Blocked by project decision to deprecate MSVC support in favor of #31507
- Unaddressed objection: adds MSVC code conflicting with #31507, no author reply (fanquake)
- Verified by testing five- to ten-fold hashing speedups on Windows (sipsorcery, m3dwards)
- Concept approval while questioning the AVX2 choice (laanwj)
| 102- 102 lines added or modified outside tests
- 0 lines added or modified in tests
- 61 lines removed in total
- 4 files, 5 commits
|
(+)#35355 ajtowns Use atomics for determining whether trace logging is enabled- Avoid overhead on high-frequency code paths when trace or debug logging is disabled
- Simplify managing trace categories for node operators via the command line and RPC
| P3 · speedup- P3 because eliminating locks on disabled log checks offers minor performance hygiene
- Thread-safe log state checks provide small efficiency gains across busy threads
- Ranking pass: Lowered sharply: its atomics part overlaps 35322 and its -trace part overlaps 34038, leaving little unique value until those are decided.
| Needs rebase- Needs rebase due to merge conflicts with multiple open logging pull requests
- Stale with no author activity for nearly four months
| 0- No review verdicts recorded
| Crickets- No reviews or comments received yet
- No contributor feedback since the author opened the draft
| 238 + 109 tests- 238 lines added or modified outside tests
- 109 lines added or modified in tests
- 320 lines removed in total
- 24 files, 12 commits
|
(+)#27260 portlandhodl Enhanced error messages for invalid network prefix during address parsing.- Provide specific error messages when an address has an invalid network prefix
- Stop misreporting wrong-network addresses as completely invalid Bech32 or Base58
| P3 · cleanup- P3 because it replaces string-based error diagnostics in the bech32 helper with an error enum
- Allows callers to handle and translate decoding failures programmatically
| Needs rebase- Review is blocked because the branch has merge conflicts and needs a rebase
| 0 | Strong- Strong concept consensus on improving address error diagnostics
- Concept approval noting real-world debugging usefulness (rkrux)
- Concept approval with assistance on tests and commit structure (Sjors, l0rinc)
- Concept or approach approval without detailed rationale (jonatack, RandyMcMillan)
| 221 + 369 tests- 221 lines added or modified outside tests
- 369 lines added or modified in tests
- 352 lines removed in total
- 11 files, 8 commits
|
(+)#34603 achow101 wallet: Fix detection of symlinks on Windows- Prevent unintended directory traversal and loops when scanning wallet paths on Windows
- Properly detect symlinks and reparse points despite MinGW runtime limitations
| P3 · platform fix- P3 after comparing with the other PRs here (assessed alone as P4): Raised P4->P3 for consistency with other real platform bug fixes; a supported-platform symlink misdetection causing recursive scans is more than cosmetic.
- P4 because it adds a narrowly scoped platform helper to filesystem utilities
- Benefit is limited to supporting the wallet directory scanning fix
| Author silent- Author silent for over 100 days
- No merge conflicts or CI failures, code is ready for review
| 0 | Positive- Generally positive sentiment with technical inquiries addressed
- Approved with a minor nonblocking request for description updates (sedited)
- Technical questions on reparse behavior and duplicate calls resolved (hebasto, luke-jr)
| 30 + 27 tests- 30 lines added or modified outside tests
- 27 lines added or modified in tests
- 40 lines removed in total
- 5 files, 4 commits
|
(+)#35302 Eunovo Silent Payments: Sending (take 2)- Allow wallet users to send BIP 352 silent payments
- Improve on-chain privacy and eliminate address reuse by generating unique unlinked taproot outputs
| P3 · new feature- P3 because it adds shared BIP 352 cryptographic wrappers in common utilities
- Provides reusable helper code for wallet operations and future indexing utilities
- Ranking pass: Lowered well below its score: its utils footprint is thin wrappers on top of 35301, which should be reviewed first.
| Needs rebase- Review #35301 first
- Needs rebase due to merge conflicts with master
| 0 | Positive- Concept approval (rkrux)
- Feedback on script handling and spend RPCs was resolved (theStack)
- Suggestions on functional tests and bumpfee test coverage were addressed (rkrux)
| 1,487 + 7,258 tests- 1,487 lines added or modified outside tests
- 7,258 lines added or modified in tests
- 114 lines removed in total
- 47 files, 18 commits
|
(+)#32966 Eunovo Silent Payments: Receiving- Enables Bitcoin Core wallet to receive BIP 352 Silent Payments
- Lets recipients publish a single static address without compromising privacy through address reuse
| P3 · new feature- P3 because subtree modifications should be maintained and merged upstream in libsecp256k1 first
- Adds cryptographic primitives and benchmark tooling needed for BIP 352
- Ranking pass: Lowered: depends on 35301 and on upstream libsecp256k1 work (#1799); utils contribution is incremental.
| Needs rebase- Needs rebase, has merge conflicts, and author has been silent for months
- Review #35301 first and wait for upstream libsecp256k1 support
| 0- No review verdicts recorded
| Strong- Strong concept consensus on adding silent payment receiving to the wallet
- Iteratively tested wallet creation and rescanning across revisions (Sjors)
- Architectural feedback on key derivation and descriptor structures (josibake)
- Contributed review feedback on tests (rkrux)
| 15,289 + 3,665 tests- 15,289 lines added or modified outside tests
- 3,665 lines added or modified in tests
- 165 lines removed in total
- 94 files, 41 commits
|
(+)#36282 fanquake Update leveldb subtree to latest master- Stop LevelDB headers from generating unused constant compiler warnings
- Unblocks enabling stricter unused constant variable checks across the project
| P4 · unblocks #36275- P4 after comparing with the other PRs here (assessed alone as P3): Lowered P3->P4 to match 36278: both are warning-only subtree syncs unblocking a compiler-flag PR, which carries no security or correctness fix.
- P3 because it unblocks enabling stricter compiler warning flags across the build
- Eliminates header warnings in vendored storage dependency code
| Ready- Ready for review as a clean and minimal subtree update
| 1 | Positive- Smooth consensus on importing the upstream changes
- Code review approval without objections (l0rinc)
| +16/-16 |
(+)#36278 fanquake Update minisketch subtree to latest master- Prevent unused-variable warnings from minisketch when compiling with -Wunused-const-variable
- Unblocks enabling the -Wunused-const-variable compiler warning across the codebase
| P4 · unblocks #36275- P4 because this is a minor upstream subtree sync with no functional or security impact
- Provides minor leverage by unblocking warning enablement in #36275
- Ranking pass: Moved up to sit beside 36282, the identical subtree sync for the same #36275 unblock.
| Ready- Ready to review
- Clean subtree bump with no conflicts or failing tests
| 0- No review verdicts recorded
| Crickets- No reviews or comments yet
- Recently opened with no discussion so far
| 4- 4 lines added or modified outside tests
- 0 lines added or modified in tests
- 1 lines removed in total
- 1 files, 2 commits
|
(+)#36033 maflcko [wip,nomerge,rfc] build: Require C++23 compiler- Eliminate custom workarounds and polyfills by requiring C++23
- Let developers use modern standard library features directly
| P4 · cleanup- P4 after comparing with the other PRs here (assessed alone as P3): Lowered P3->P4: author-marked wip/nomerge RFC toolchain bump; speculative and weakly justified on its own terms.
- P3 because replacing custom utility polyfills with standard equivalents is routine cleanup
- Simplifies internal infrastructure slightly without changing developer workflows significantly
| Needs rebase- Not ready for review because the PR has merge conflicts and CI is failing
| 0 | Positive- Initial sentiment is positive on exploring the compiler upgrade
- Concept approval without stated reasons (l0rinc)
- Minor feedback on header include heuristics (hebasto)
| 69 + 43 tests- 69 lines added or modified outside tests
- 43 lines added or modified in tests
- 280 lines removed in total
- 50 files, 8 commits
|
(+)#31672 vasild rpc: add cpu_load to getpeerinfo- Help node operators and developers measure CPU time spent processing messages from individual peers
- Identify resource-intensive peers to assist with peer prioritization and DoS investigations
| P4 · new feature- P4 because the thread timing helpers are introduced solely to support this one RPC metric
- Has no immediate demand or utility across other areas of the codebase
| Author silent 141 days- Worthwhile to review architectural direction, though the author has been inactive for several months
| 1 (+1) -1 | Blocked- Concept support for monitoring per-peer CPU usage to assist anti-DoS measures (laanwj)
- Prefers an experimental RPC with a decaying average over adding this directly to getpeerinfo (sipa)
- Unaddressed objection: CPU load is not actionable and does not distinguish useful work (ajtowns)
- Unaddressed objection: CPU load alone is useless without transaction context (rebroad)
| 159 + 1 tests- 159 lines added or modified outside tests
- 1 lines added or modified in tests
- 4 lines removed in total
- 8 files, 2 commits
|
(+)#35315 maflcko refactor: Use NodeClock::time_point in more places- Unify time handling across P2P code to eliminate confusion from mixing raw integers and time points
- Remove deprecated time helpers to make timing mistakes harder in net processing
| P4 · cleanup- P4 because changes to time utilities are cosmetic refinements and docstring additions
- Does not substantially alter underlying time primitives
| Needs rebase- Needs rebase due to merge conflicts with master
- Author has been silent for nearly two months with recent nits unaddressed
| 3 (+2) | Positive w/ caveats- Broad support for type safety improvements across time handling
- Approves the code while noting expanding epoch sentinel usage is suboptimal (ryanofsky)
- Withdrew nonblocking preference for std::optional given overall improvements (stickies-v)
- Verified by testing with minor nits (jeanpablojp)
- Concept approval without stated reasons (w0xlt)
| 148 + 35 tests- 148 lines added or modified outside tests
- 35 lines added or modified in tests
- 181 lines removed in total
- 24 files, 10 commits
|
(+)#34730 ajtowns util/log: Combine the warning/error log levels into a single alert level- Remove ambiguity between warning and error log levels for developers and node operators
- Eliminate a historical distinction that node operators already observe directly
| P4 · cleanup- P4 because it is a minor ergonomic cleanup to internal logging levels
- Offers cosmetic consistency across call sites without fixing bugs or stability issues
| Needs rebase- Needs rebase due to merge conflicts with other open logging changes
- Review #34038 first
| 1 | Positive w/ caveats- Code review approval despite reservations about the chosen design (ryanofsky)
- Prefers keeping warning and error separate while adding a critical level (ryanofsky)
| 48 + 15 tests- 48 lines added or modified outside tests
- 15 lines added or modified in tests
- 57 lines removed in total
- 7 files, 2 commits
|
(+)#35461 optout21 util: Clarify the assertion message in assertion failures (Assert, Assume, etc.)- Make assertion failure messages unambiguous for developers debugging failures
- Ensure nonfatal unreachable checks honor the test abort-on-assume setting
| P4 · cleanup- P4 because the benefit is a minor diagnostic clarity fix for developers
- Affects only internal failure reporting and test configuration consistency
| Ready- Ready to review
- Clean merge and all reviewer suggestions have been incorporated
| 1 | Strong- Strong consensus with all suggested changes resolved
- Tested and approved following wording and formatting adjustments (l0rinc)
- Supported approach and helped refine unreachable abort helper reuse (maflcko)
| 31 + 21 tests- 31 lines added or modified outside tests
- 21 lines added or modified in tests
- 14 lines removed in total
- 4 files, 3 commits
|
(+)#36038 fanquake util: remove usage of `strerror_s/r`- Simplify operating system error code formatting across platforms
- Remove custom platform branching and build configuration checks
| P4 · cleanup- P4 because it is an internal cleanup with no performance, stability, or user-visible impact
- Simplifies cross-platform error string handling while keeping behavior effectively unchanged
| Ready- Ready for review: passes CI and has no unresolved questions
| 0- No review verdicts recorded
| Crickets- No substantive reviews or concept comments yet
- Resolved a documentation formatting nit (maflcko)
| 7- 7 lines added or modified outside tests
- 0 lines added or modified in tests
- 67 lines removed in total
- 8 files, 2 commits
|
(+)#17581 ryanofsky refactor: Remove settings merge reverse precedence code- Simplify settings evaluation logic by removing obsolete legacy precedence handling
- Eases maintenance of configuration parsing once duplicate settings are disallowed
| P4 · cleanup- P4 because it is an internal cleanup removing redundant settings parsing code
- Does not alter user-facing behavior once prerequisite pull requests land
| Review #17493 first- Review #17493 first
- Contains one unique commit sitting on top of a stack of prerequisite pull requests
| 0- No review verdicts recorded
| Crickets- No reviewer feedback or discussion yet
- The author remains active and regularly rebases the stack
| 389 + 811 tests- 389 lines added or modified outside tests
- 811 lines added or modified in tests
- 223 lines removed in total
- 18 files, 19 commits
|
(+)#35170 optout21 test: Better test coverage for legacy ParseHDKeypath()- Verify actual parsed keypaths in unit tests instead of only checking success booleans
- Clarify in the codebase that this keypath parsing function is legacy wallet code
| P4 · test coverage- P4 because it adds incremental test assertions and a cosmetic rename to a legacy utility
- Modifies no production behavior and carries no user-facing impact or downstream leverage
- Ranking pass: Test-only assertions plus a rename; no production behavior at all, so it sits below the other P4 cleanups.
| Ready- Ready to review: clean diff against master with passing CI and resolved reviewer feedback
| 0 (+2) | Strong- Strong support for testing parsed output values and clarifying legacy status
- Verified by local testing of test coverage and rename (haishmg)
- Code review approval after reviewing test cases (brunoerg)
- Concept approval without stated reasons (l0rinc, w0xlt)
| 4 + 123 tests- 4 lines added or modified outside tests
- 123 lines added or modified in tests
- 103 lines removed in total
- 6 files, 5 commits
|
(+)#33646 cedwies log: check fclose() results and report safely in logging.cpp- Prevent unnoticed diagnostic loss by reporting errors when closing log files
- Report close failures directly to stderr during log rotation, shrinking, or shutdown
| P4 · cleanup- P4 because log files disable stream buffering, so writeback errors surface on write rather than close
- Defensive error reporting provides only a minor benefit for unexpected close failures
| Ready- Ready for review as code is compact and open feedback consists only of minor nits
| 0- No review verdicts recorded
| Positive- General agreement on defensive error checking (sedited, maflcko)
- Noted that unbuffered writes make errors surface early, confirming the change is mainly defensive (maflcko)
- Minor style nits and commit message clarification pending author response (sedited)
| 29- 29 lines added or modified outside tests
- 0 lines added or modified in tests
- 8 lines removed in total
- 1 files, 1 commits
|
(+)#31868 l0rinc [IBD] specialize block serialization- Reduce overhead when serializing single bytes and computing serialized data sizes
- Speed up block serialization and initial block download
| P4 · speedup- P4 because real-world performance gains remain speculative
- Existing buffered streams already prevent single-byte writes during IBD, making gains marginal
- Adds template and code complexity without verified practical speedups
| Needs rebase- Needs rebase due to merge conflicts
- Author is holding local revisions and has been inactive
| 0- No review verdicts recorded
| Mild- Mild skepticism about real-world performance gains versus added complexity
- Questions whether production IBD hits these paths given existing buffered I/O (maflcko)
- Unable to reproduce claimed benchmark performance gains (sedited)
- Author acknowledged the added complexity and plans to retest local changes (l0rinc)
| 196 + 38 tests- 196 lines added or modified outside tests
- 38 lines added or modified in tests
- 98 lines removed in total
- 11 files, 5 commits
|