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

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

> Can you please point me to a documentation or code which demonstrates that?

Not an expert on this, but seeing the note about `uintptr_t` at https://en.cppreference.com/w/cpp/language/reinterpret_cast.html is what prompted me to post the suggestion:

> If the implementation provides [std::intptr_t](https://en.cppreference.com/w/cpp/types/integer.html) and/or [std::uintptr_t](https://en.cppreference.com/w/cpp/types/
...

💬 ariard commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3099081175)
> This is already possible today. Besides i don't see how this is an issue: an untrusted counterparty can always make your
> funding protocol fail by providing invalid or non-standard inputs, or even just stopping to respond for that matter.

I don't think we're disagreeing that this new `MAX_TX_LEGACY_SIGOPS` can be a source of denial-of-service for collaborative transaction construction. But the point is that policy limits are already encompassed by second-layers flow to _sanitize_ invalid
...

🤔 murchandamus reviewed a pull request: "wallet, rpc: add anti-fee-sniping to `send` and `sendall`"
(https://github.com/bitcoin/bitcoin/pull/28944#pullrequestreview-3039897102)
ACK aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f

Compared to my prior ACK via range_diff, and only found this minor change and some changes due to the rebase.

```diff
- can_anti_fee_snipe &= (tx_in.nSequence == CTxIn::MAX_SEQUENCE_NONFINAL || tx_in.nSequence == MAX_BIP125_RBF_SEQUENCE);
+ can_anti_fee_snipe = can_anti_fee_snipe && (tx_in.nSequence == CTxIn::MAX_SEQUENCE_NONFINAL || tx_in.nSequence == MAX_BIP125_RBF_SEQUENCE);
```

Tested that the new tests fail without the first com
...

📝 achow101 opened a pull request: "wallet: Set descriptor cache upgraded flag for migrated wallets"
(https://github.com/bitcoin/bitcoin/pull/33031)
#32597 set the descriptor cache upgraded flag for newly created wallets, but migrated wallets still did not have the flag set when they are migrated. For consistency, and to avoid an unnecessary upgrade, we should be setting this flag for migrated wallets.

The flag would end up being set anyways at the end of migration when the wallet is reloaded as it would perform the automatic upgrade at that time. However, this is unnecessary and we should just set it from the get go.

This PR also adds
...

📝 achow101 opened a pull request: "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase"
(https://github.com/bitcoin/bitcoin/pull/33032)
`MockableDatabase` was introduced for the tests to avoid tying non-database tests to a particular database type. However, since the only database type now is sqlite, and because the mockable behavior is no longer used by the tests, we can replace usage of the `MockabeDatabase` with a SQLite database that lives only in memory.

This is particularly useful for future work that has the wallet make use of SQLite's capabilities more, which are less conducive to having a separate mock database imple
...

📝 achow101 opened a pull request: "wallet, sqlite: Encapsulate SQLite statements in a RAII class"
(https://github.com/bitcoin/bitcoin/pull/33033)
SQLite statements are C objects which can be long lived, so having them be initialized in a constructor, and cleanup everything in a destructor, makes the sqlite code cleaner and easier to read. This also lets us have the various functions needed to interact with sqlite statements be member functions.

This will be particularly useful in future work which makes use of more complicated SQL statements.