bitcoin icon indicating copy to clipboard operation
bitcoin copied to clipboard

refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags

Open ryanofsky opened this issue 6 years ago • 7 comments

This is based on #16545. The non-base commits are:


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

ryanofsky avatar Nov 24 '19 20:11 ryanofsky

Concept ACK

hebasto avatar Nov 24 '19 20:11 hebasto

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 -loadblock by 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

DrahtBot avatar Nov 24 '19 23:11 DrahtBot

~~Wrong commit bcbbc48a1fe7273494738200d508affef40cb47b?~~ nevermind.

Concept ACK.

promag avatar Dec 22 '19 22:12 promag

Moved this to draft for now, given it's based on another PR, which itself is a draft.

fanquake avatar Feb 08 '23 11:02 fanquake

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

ryanofsky avatar Dec 06 '24 16:12 ryanofsky

🚧 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.

DrahtBot avatar Dec 06 '24 16:12 DrahtBot

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot avatar Nov 21 '25 23:11 DrahtBot