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

📝 achow101 opened a pull request: "wallet: Store transactions in a separate sqlite table"
(https://github.com/bitcoin/bitcoin/pull/33034)
The wallet uses SQLite as a key-value store even though SQLite is a powerful relational database engine. This causes us numerous headaches due to the need to serialize multiple fields together, and since record read from the database during loading can come in any order.

Notably, for transactions, if we were able to load them in the transaction order assigned by the wallet, PRs like #27865 would be a bit simpler and easier to reason about.

This PR makes it possible for us to do that by sto
...

🤔 murchandamus reviewed a pull request: "wallet, rpc: add v3 transaction creation and wallet support"
(https://github.com/bitcoin/bitcoin/pull/32896#pullrequestreview-3040093165)
Concept ACK

💬 murchandamus commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2220548538)
Following the prior comment, I think it would be good to have a test where you are trying to build a transaction for which the version is not set, that has both a pre-selected version 1|2 and a pre-selected version 3 input.

💬 murchandamus commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2220508773)
In "wallet: throw error at conflicting tx versions in pre-selected inputs" (cf712ab56e3256db5dfcf422860bdb3cf87cd03e):
Is `m_version` always set?
Otherwise, what would happen if `coin_control.m_version` is not set, and one preselected input is TRUC, but another one is not?

💬 murchandamus commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2220523528)
In "wallet: don't include unconfirmed v3 txs with children in available coins" (bff006b7ae7fa120fac4f2118e891d2c88d38f9a):

I find this comment hard to parse. Do you mean something along the lines of:

```suggestion
// this marks all outputs of any unconfirmed v3 parent transactions as unspendable
// if any of its outputs are spent by this transaction
```