💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2220043055)
The test you propose is also interesting, and someone might want to take a stab at adding it in another pull request. It is out of scope for this pull request however.

Fuzzing explores pseudorandomly all code paths that can be reached by exercising the fuzz harness. By comparing BnB with a brute force search, we can demonstrate that BnB always finds the best solution that is present, but only when they are present. As only inputs that cause new paths to be explored in the source code are reta
...

💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#issuecomment-3098071484)
Removed the duplicate definition of `coin_params.m_cost_of_change`.

💬 mzumsande commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2219983800)
I wonder if it wouldn't be simpler if all of the `m_block_xyz` class members were just local variables within `CoinStatsIndex::CustomAppend` instead.
The previous values are being overwritten here with each new block, and as far as I can see the saved values (while being set to correct values in `CustomInit` and `ReverseBlock`) are not used anywhere - except for
[this line](https://github.com/fjahr/bitcoin/blob/4cb2c0520211634ee3caf6d4c3a1e65b2bdb2dc6/src/index/coinstatsindex.cpp#L483C5-L483C
...

👍 ryanofsky approved a pull request: "rpc: Handle -named argument parsing where '=' character is used"
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3039323143)
Code review ACK 56b1da77b5998c44b3bb799689a6c0449e9223fc. Thanks for the update! The fix here seems right and should be practically useful. And the new tests seem to cover different scenarios where positional string parameters contain '='.

Two comments:

- I think it would good to add a test to cover cases where positional *non-string* parameters contain '=', i.e. the `bitcoin-cli -named echojson '["fail=yes"]'` case which [previously failed](https://github.com/bitcoin/bitcoin/pull/32821#di
...

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2220035865)
In commit "test: Add functional tests for named argument parsing" (56b1da77b5998c44b3bb799689a6c0449e9223fc)

Note: this comment is not exactly describing what is happening during this test. It's not true in general that unknown parameters with `=` are treated as positional. Unknown parameters with `=` are only treated as positional if the next positional parameter is known and either accepts a string (this case) or if it's known and accepts JSON and the argument actually parses as json. Other
...

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2220009660)
re: https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2203147913

Thanks! it appears all issues are solved now with the latest push (56b1da77b5998c44b3bb799689a6c0449e9223fc)

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2220003117)
In commit "rpc: Handle -named argument parsing where Base64 encoding is used" (e20cba675bfa5e17da1a0776dda0070e9c94aff1)

Not sure the names `is_string_method` and `is_convert_method` are really accurate here. `is_string_method` is true if the method's next positional argument expects a string, and `is_convert_method` is true if it expects a non-string value. Could maybe consider calling them `next_positional_arg_is_string` or `next_positional_arg_is_json` or something like that. Code looks li
...

💬 l0rinc commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3098279743)
ACK 2a8fdddd4df4b630c0a580f4df6521cb3af01804

Ran the tests a few times, before:
> 139/140 Test #29: coins_tests .......................... Passed 71.43 sec

and
> 138/140 Test #29: coins_tests .......................... Passed 60.65 sec

After:
> 66/142 Test #31: coins_tests .......................... Passed 10.86 sec
> 128/142 Test #29: coins_tests_base ..................... Passed 23.53 sec
> 138/142 Test #30: coins_tests_dbbase ................... Passed 4
...

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#issuecomment-3098376004)
Rebased after https://github.com/bitcoin/bitcoin/pull/32521/files#diff-ea6d307faa4ec9dfa5abcf6858bc19603079f2b8e110e1d62da4df98f4bdb9c0R184

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3098588297)
> Another thing I noticed while testing using the `send` RPC, in a wallet with _only_ an `sp()` descriptor, is that it insists on having a bech32 descriptor for change:
>
> > Transaction needs a change address, but we can't generate it. Error: No bech32 addresses available.

In https://github.com/bitcoin/bitcoin/pull/32966/commits/ed7f894fc0c1ed3a84f4af73ece619698cee11c0 I modify the change `OutputType` determination logic to try to use silent payments if `bech32m` addresses are not availab
...

🤔 ismaelsadeeq reviewed a pull request: "index: initial sync speedup, parallelize process"
(https://github.com/bitcoin/bitcoin/pull/26966#pullrequestreview-3039751954)
Concept ACK, did a quick parse through this.

Will do in-depth review soon

> Now the juicy part:

What is the step to reproduce your result?

Side note to self could be useful to try benchmarking this using [benchkit](https://github.com/bitcoin-dev-tools/benchkit) so that anyone can reproduce using the yaml config?

💬 ismaelsadeeq commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2220282853)
In "util: introduce general purpose thread pool" 7d984c3b2dca085ef7f49d21568b06c7b89c7807

Hmm I can imagine that this can be blocking; but I guess there is a reason why you did not return here and then empty the work queue in Stop.
```suggestion
// If stopped and no work left, exit worker.
```

💬 ismaelsadeeq commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2220285854)
In "util: introduce general purpose thread pool" 7d984c3b2dca085ef7f49d21568b06c7b89c7807

I think this and other verbose comment can be removed, it quite obvious.

What might need comment is Submit due to the abstraction there.

💬 ismaelsadeeq commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2220317691)
In " init: provide thread pool to indexes " 82fa9d29653b445118fc2d03e2ced520a5d4c7dc

Define the max and then use it here and in checking the bounds

💬 ismaelsadeeq commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2220319189)
In " init: provide thread pool to indexes " 82fa9d29653b445118fc2d03e2ced520a5d4c7dc

Why 0?

💬 ismaelsadeeq commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2220304297)
In "util: introduce general purpose thread pool" 7d984c3b2dca085ef7f49d21568b06c7b89c7807

Also verify that no work queue size is 0.

💬 ismaelsadeeq commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2220315413)
In " init: provide thread pool to indexes " 82fa9d29653b445118fc2d03e2ced520a5d4c7dc

This message should be more verbose users should know the limit

💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2220314059)
re: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217039648

> Not sure I understand why this is better, we have the xor instructions spread to multiple places now - apply_random_xor_chunks & inline `original ^ key_bytes`

Hmm. Are we looking at the same diff? Below is an updated diff based on master. It is net -26 lines of code and has exactly one xor operation. It makes the simple test stronger, deletes the more complicated test, testing all the same cases and providing all th
...

💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2220219737)
re: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217028864

> Did that before, but @hodlinator pointed out that the previous version made more sense since it's storing obfuscation key's database key: [#31144 (comment)](https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2068663871).

Yes, but that was when the row contained was read as a key rather than an Obfuscation object. IMO, if the row contains a serialized Obfuscation object the key should be called OBFUSCATION_KEY
...

💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2220169264)
re: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217024259

> I don't think so, `Write` needs `m_obfuscation`

Thanks, a comment that says something like "m_obfuscation must be unset while calling Write() so Write() does not try to obfuscate the obfuscation key" would be helpful. I still don't think the unnecessary `Read()` call here is good, but at least I see why it's being done now.

Actually, the main thing I don't like about current code is the way it calls `Write` with
...