💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727790062)
Just spotted the whitespace at end of line.. fixing. :)

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727794829)
("ParseHex()" becomes a link in Doxygen).

💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2305609525)
ACK df92661444f46790b12d5061344d72106ef820d9

Doc updates

💬 theStack commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1727850357)
Not sure if there are guidelines for this (and if yes, how strict we are), but I would tend to avoid using the "property" decorator for non-trivial methods that access state outside of the class (in this case, by interacting with the file system). At least I wouldn't expect as user that accessing a class field ever involves any I/O. I think dropping the property and keeping the name as-is "read_xor_key" might be just fine?

💬 theStack commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1727856174)
nit: could put this already into the commit that introduces the blocks_key_path property to TestNode.

🤔 furszy reviewed a pull request: "init: fix init fatal error on invalid negated option value"
(https://github.com/bitcoin/bitcoin/pull/30684#pullrequestreview-2255865283)
Hmm yeah ok, nice catch. I missed that `getSettingsList` return a list of json that can be checked against the string type.
One drawback of your suggestion is that we currently fail (crash) when the invalid value is provided and with your suggestion, we would ignore it. So, by following-up this fix with new args restrictions (which I think we should do) we would be changing behavior twice. Unless we don't backport the crash fix and merge both PRs during the same release window or.. we fail with
...

📝 martinsaposnic opened a pull request: "Use MiniWallet in functional test rpc_signrawtransactionwithkey."
(https://github.com/bitcoin/bitcoin/pull/30701)
In response to issue https://github.com/bitcoin/bitcoin/issues/30600, optimizations have been implemented to enhance test efficiency and readability:

1. Simplified `send_to_address` Method:

The `send_to_address` method has been refactored to utilize MiniWallet. This change significantly reduces unnecessary complexity and improves performance. By abstracting away the low-level transaction details of funding transaction creation, the test code becomes more concise and easier to understand.

...

💬 martinsaposnic commented on issue "use MiniWallet in functional test `rpc_signrawtransactionwithkey.py` for funding txs":
(https://github.com/bitcoin/bitcoin/issues/30600#issuecomment-2305817413)
new possible solution for this issue: https://github.com/bitcoin/bitcoin/pull/30701 🙂

💬 furszy commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2305829414)
Found #16545 at the `ArgsManager::Flags` enum when started implementing the same idea. Cool stuff. Happy to move there on this release cycle. Will report a general error at the two `if (!wallet.isStr())` lines to fix the crash and keep the init failure here.

💬 ryanofsky commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2305863821)
I'm not sure I understand the drawback of the suggested fix in https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2299869398. You are concerned that a writing `-nowallet=0` or `-nowallet=not_a_bool` would be ignored instead of treated as an error, and then potentially be treated as an error again at some future date?

This doesn't seem like big problem to me because `-nosetting=0` `-nosetting=not_a_bool` is already allowed most other places and it is just treated `-setting=1`. So it se
...

💬 furszy commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2305881082)
> I'm not sure I understand the drawback of the suggested fix in [#30684 (comment)](https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2299869398). You are concerned that a writing `-nowallet=0` or `-nowallet=not_a_bool` would be ignored instead of treated as an error, and then potentially be treated as an error again at some future date?

Yeah, I'm just trying to avoid users placing something like this in the config file, forgetting about it, and then coming back in the future (after
...

🤔 theStack reviewed a pull request: "Use MiniWallet in functional test rpc_signrawtransactionwithkey."
(https://github.com/bitcoin/bitcoin/pull/30701#pullrequestreview-2255954132)
Thanks for picking this up!

A few more suggestions to reduce the code even further:
* the initialization of previously needed instance members (`self.blk_idx`, `self.block_hash`) can now be removed
* instead of starting with a clean chain (see `self.setup_clean_chain`), you could use the pre-generated test chain to avoid the need of generating blocks; MiniWallet will pick the relevant UTXOs up automically at instantiation. IIUC it shouldn't be necessary to mine any blocks for this tests
*
...

💬 kevkevinpal commented on pull request "CI: fix 3 simple codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#issuecomment-2305907411)
Can you update the title and commit to start with `doc:` instead of `CI:`?

since this is just a doc change

💬 kevkevinpal commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2305923063)
I don't understand why we need to refactor the whole test, I think additional asserts I'm also neutral on

I think adding asserts without refactoring the test would be better

🤔 ryanofsky reviewed a pull request: "refactor: Implement missing error checking for ArgsManager flags"
(https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2256044988)
Updated 1e37bcf9fc11562baaedea24685c31f60ef2de31 -> 41bdf3d025f900a59ec14d5b497a31a2d84eea52 ([`pr/argcheck.39`](https://github.com/ryanofsky/bitcoin/commits/pr/argcheck.39) -> [`pr/argcheck.40`](https://github.com/ryanofsky/bitcoin/commits/pr/argcheck.40), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/argcheck.39..pr/argcheck.40)) dropping support for flag combinations and fixing a commit message as suggested.

Support for flag combinations is added back in commit df5c3e123227c1c
...

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1728104674)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1706731123

> I agree, but I don't think it is incompatible with my suggestion. It seems possible to just implement bool error checking (and only that, or any other single feature), and make sure it is coherent and will be used, and then roll out the feature to one bool-only setting at a time, changing the behavior only once for this setting.

Thanks for pushing this, and that approach does sound ok to me. I think your advice to s
...

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1728104758)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1718228472

> The example for `ALLOW_INT | ALLOW_BOOL` in 6865a198f5db30bd494b3a2540f47ee728963908 is a bit worrying.

Thanks, and yes that example is very out of date. I removed it from the PR description. Now that std::optional GetArg overloads exist, there is little reason to use the other GetArg functions, and their behavior has also been simplified. The way this would be written with the current PR would look more like the `-
...

👍 ryanofsky approved a pull request: "init: fix init fatal error on invalid negated option value"
(https://github.com/bitcoin/bitcoin/pull/30684#pullrequestreview-2256090693)
Code review ACK e56e2f51803b276945c74a479d30d884256d7ffc.

Thanks for the fix! Could update the description and note that this bug was caused by #22217. Before that PR specifying -nowallet=0 or -nowallet=not_a_bool would be equivalent to specifying -wallet=1, after that PR it triggers an uncaught exception, and now it triggers a clearer error. I think this error could also be triggered by editing `settings.json` and adding a non-string wallet name.

💬 ryanofsky commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#discussion_r1728138715)
In commit "init: fix fatal error on '-wallet' negated option value" (e56e2f51803b276945c74a479d30d884256d7ffc)

If desired, you could include the value in the error message like `strprintf(_("Invalid -wallet value %s detected"), wallet.write())`

👍 ryanofsky approved a pull request: "refactor: Replace ParseHex with consteval ""_hex literals"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2256099298)
Code review ACK df92661444f46790b12d5061344d72106ef820d9. Just documentation update since last review (thanks!)