bitcoin
bitcoin copied to clipboard
refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags
This is based on #16545. The non-base commits are:
da3bdd5f00b3common: Add support for combining ArgsManager flags603423e13966scripted-diff: Add ALLOW_LIST flag to arguments retrieved with GetArgs8f6aa8575f6frefactor: Fix more ALLOW_LIST arguments7a756355c1deAlways reject empty -blockfilterindex="" argumentsd2edef6b1a0brefactor: Always enforce ALLOW_LIST in CheckArgFlags
Except for two rpcauth and blockfilterindex fixes (update: rpcauth fix was merged separately in #30401) this PR does not change any behavior outside of tests. It is just supposed to enforce internal consistency and prevent bugs by ensuring that list arguments are always retrieved with GetArgs() and non-list arguments are always retrieved with GetArg(). Followup PRs could use the ALLOW_LIST flags for better documentation and error checking in the future. For example, #17493 builds on this to disallow conflicting config values.
This change was originally made as part of #17493
Concept ACK
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Code Coverage & Benchmarks
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/17580.
Reviews
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | hebasto, promag, ajtowns |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #33920 (Export embedded ASMap RPC by fjahr)
- #33892 (policy: Remove individual transaction <minrelay restriction by instagibbs)
- #33770 (init: Require explicit -asmap filename by ryanofsky)
- #33631 (init: Split file path handling out of -asmap option by fjahr)
- #33629 (Cluster mempool by sdaftuar)
- #33343 (help: enrich help text for
-loadblockby HowHsu) - #32297 (bitcoin-cli: Add -ipcconnect option by ryanofsky)
- #32138 (wallet, rpc: remove settxfee and paytxfee by polespinasa)
- #31974 (Drop testnet3 by Sjors)
- #31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
- #28792 (build: Embedded ASMap [3/3]: Build binary dump header file by fjahr)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
LLM Linter (✨ experimental)
Possible typos and grammar issues:
- useable → usable [“useable” is a less common variant; “usable” is standard]
- canoot → cannot [typo in “canoot be negated”]
drahtbot_id_4_m
~~Wrong commit bcbbc48a1fe7273494738200d508affef40cb47b?~~ nevermind.
Concept ACK.
Moved this to draft for now, given it's based on another PR, which itself is a draft.
Dropping commit 811670650471b352b5e0755e6f38b85154c2bfb3 which is no longer needed after #30401
Rebased a1d65066621a21ea35c4ec8363cfabb862fb5239 -> 8723b8ed56a03394663945bcdaa00039ccdff5f6 (pr/wdlist.19 -> pr/wdlist.20, compare) due to various conflicts
Updated 8723b8ed56a03394663945bcdaa00039ccdff5f6 -> d2edef6b1a0bde00a29290229a5964e413c49696 (pr/wdlist.20 -> pr/wdlist.21, compare) adding commit da3bdd5f00b3dcd380b7bd67d2d54ec7c4b9c2bb which previously was part of base PR #16545 but was dropped there to make review easier. This functionality in this commit is needed for the -blockfilterindex argument
Rebased d2edef6b1a0bde00a29290229a5964e413c49696 -> 077e20ffba77ec70b89f12df1a7d47c836fb5a67 (pr/wdlist.21 -> pr/wdlist.22, compare) due to conflicts with #28358, #31223, and #31483
Rebased 077e20ffba77ec70b89f12df1a7d47c836fb5a67 -> 8f2544b1be1faaf1932b0fe87e4f52c76a8cdaba (pr/wdlist.22 -> pr/wdlist.23, compare) due to conflicts
Updated 8f2544b1be1faaf1932b0fe87e4f52c76a8cdaba -> b7402367abfc8bd3ee3a21205b96e940223c6cdc (pr/wdlist.23 -> pr/wdlist.24, compare) due to silent conflict with #32425
Updated b7402367abfc8bd3ee3a21205b96e940223c6cdc -> 0c5171ff029d1bf871aef1c5a48863ee7aaf0e3a (pr/wdlist.24 -> pr/wdlist.25, compare) to fix wallet tool test failure cause by bad conflict resolution in push before last
Rebased 0c5171ff029d1bf871aef1c5a48863ee7aaf0e3a -> 973220fe6c55f8bb5c724b0f224c9f1942dde795 (pr/wdlist.25 -> pr/wdlist.26, compare) to fix for-each-commit merge errors
Updated 973220fe6c55f8bb5c724b0f224c9f1942dde795 -> 14f3c77fef85eedb5abc5297f4761e587acacd6d (pr/wdlist.26 -> pr/wdlist.27, compare) adding another -proxy fix for #17783
Rebased 14f3c77fef85eedb5abc5297f4761e587acacd6d -> 6e874dee28772b6a9fc1453b029ff2a099d8bfd1 (pr/wdlist.27 -> pr/wdlist.28, compare) due to for-each-commit merge error https://github.com/bitcoin/bitcoin/actions/runs/16686593625/job/47237155434?pr=17580
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34042330155
Hints
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
-
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
-
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
-
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
🐙 This pull request conflicts with the target branch and needs rebase.