🤔 w0xlt reviewed a pull request: "wallet: Replace CWalletTx::mapValue and vOrderForm with explicit class members"
(https://github.com/bitcoin/bitcoin/pull/32763#pullrequestreview-2976772441)
ACK https://github.com/bitcoin/bitcoin/pull/32763/commits/9a8fd16e67755e336de683c0158d4fdd2b4ddfe8.
`value_map` and `order_form` (and therefore `WalletOrderForm` and `WalletValueMap`) lack clarity, making replacing with well-defined fields a much better approach.

🤔 ismaelsadeeq reviewed a pull request: "refactor: CFeeRate encapsulates FeeFrac internally"
(https://github.com/bitcoin/bitcoin/pull/32750#pullrequestreview-2976133077)
The current iteration is looking better, I will do another round of review and testing tomorrow.

Just a few nits after a short glance

💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2178028452)
nit: be less ambiguous?
```suggestion
const CAmount feerate_per_kvb = GetFeePerK();
```

💬 ismaelsadeeq commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2178446483)
nit:
includes are alphabetical

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2178453208)
Also, RPC `migratewallet` calls the same function so I could refactor that instance too.

💬 achow101 commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3025413663)
> The approach I suggested of just backing up all wallets as `{prefix}_{timestamp}.legacy.bak` files in the top-level wallets directory seems like it should make the backup files easier to find, identify, and clean up.

That seems reasonable to me.

💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-3025430544)
> Would there be interest in adding a benchmark for the Trim() operation here, or rather in a follow-up?

That will be nice, I don't mind adding it here. I plan to do another round of review of the added tests and diff after my last review so if added will review it as well.

I try running the bench on the linked commit. `MakeTxGraph` seems to takes two parameters, but the bench call provided three arguments.
I removed `NUM_ACCEPTABLE_ITERS` arg this is the result.


| ns/o
...

💬 achow101 commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#issuecomment-3025443426)
ACK 8cc9845b8ddf4f93a02c622e7df8d1095dc1a640

💬 achow101 commented on pull request "wallet, rpc: Use `OUTPUT_TYPES` to describe the output types instead of hardcoding them":
(https://github.com/bitcoin/bitcoin/pull/32432#issuecomment-3025445225)
ACK 8cc9845b8ddf4f93a02c622e7df8d1095dc1a640

💬 achow101 commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#issuecomment-3025474575)
ACK 941b8f54c0d35d3243bb6083f3b52681d1b9a555

🚀 achow101 merged a pull request: "wallet, rpc: Use `OUTPUT_TYPES` to describe the output types instead of hardcoding them"
(https://github.com/bitcoin/bitcoin/pull/32432)

💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2178494285)
It can move backwards in height if reorg goes across difficulty adustments (impossible on regtest) or if we mine blocks with varying MTP and use time-based timelock on the tx

🚀 achow101 merged a pull request: "test: enabling wallet migration functional test on windows"
(https://github.com/bitcoin/bitcoin/pull/32219)

💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-3025529620)
@ismaelsadeeq Yeah, the benchmark is in my "all txgraph" branch which also includes #32263. Removing the `NUM_ACCEPTABLE_ITERS` variable should be fine.

💬 achow101 commented on pull request "Wallet: "listreceivedby*" fix":
(https://github.com/bitcoin/bitcoin/pull/30972#issuecomment-3025543790)
Concept ACK

💬 achow101 commented on pull request "Refactor: Redefine CTransaction equality to include witness data":
(https://github.com/bitcoin/bitcoin/pull/32723#issuecomment-3025611532)
ACK 6efbd1e1dcdfbe9eae2d5c22abab3ee616a75ff2

🚀 achow101 merged a pull request: "Refactor: Redefine CTransaction equality to include witness data"
(https://github.com/bitcoin/bitcoin/pull/32723)

💬 davidgumberg commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2178626637)
```suggestion
for (const auto& [coin, persistent] : m_locked_coins) {
if (!success) break;
if (persistent) success = batch.EraseLockedUTXO(coin);
}
m_locked_coins.clear();

💬 achow101 commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3025700501)
In be3435c2ff82f69db11ba032452614404dac15f9 "bench: make ObfuscationBench more representative", the commit message states

> Since a previous PR already solved the tiny byte-array xors during serialization, we're only concentrating on big continuous chunks now.

and the commit essentially removes the previous benchmark. However, I don't think that's a good justification for removing the benchmark. Benchmarks for things that are "resolved" should still be kept around in order to check for reg
...

💬 Ronlabrie7470 commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-3026129000)
> Currently the *.dat files in the blocksdir store the data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan them and move them into quarantine, or delete them, or corrupt them. This may cause Bitcoin Core to fail a reorg, or fail to reply to block requests (via P2P, RPC, REST, ...).
>
> Fix this, similar to https://github.com/bitcoin/bitcoin/pull/665
...