👍 ryanofsky approved a pull request: "wallet: batch erase procedures and improve 'EraseRecords' performance"
(https://github.com/bitcoin/bitcoin/pull/29403#pullrequestreview-1873039395)
Code review ACK 8330145ba88b39ddaa9e0bbfc2d1c198da652249. Looks pretty good, but would suggest cleaning up some things

💬 ryanofsky commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484725360)
re: https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483514809

In commit "wallet: bdb batch 'ErasePrefix', do not create txn internally" (96eb527fad0effe082b087c0898843feccc2e213)

The new comment "Additionally, read-write operations performed outside a db txn context are disallowed by current BDB configurations" seems vague to me and I don't think would be comprehensible without reading this review thread. Would suggest just using achow's comment or combining both comments with s
...

💬 ryanofsky commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484711516)
In commit "wallet: db, introduce 'RunWithinTxn()' helper function" (b6dcfca8f77f32cb1f41230fb71bab6666252041)

Would be useful to add this is determines whether to commit or roll back the txn

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1484737221)
done

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1484737288)
done

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1484737478)
added note

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1484737550)
done

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1484737616)
done

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1484737693)
removed

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1484737759)
done

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1484737833)
done

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1484742841)
> Why check these values indirectly instead of directly (field `txcount`)?

Wow, I did not notice that field 🤦. That will make the test a lot better.

💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1484756018)
max_selection_weight is also fine I think, both are better than status quo

🤔 sipa reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1873140538)
ACK 13161ecf032b7a850686e5942c12222c8f3d0d52

💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484764886)
> re: [#29403 (comment)](https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483514809)
>
> In commit "wallet: bdb batch 'ErasePrefix', do not create txn internally" ([96eb527](https://github.com/bitcoin/bitcoin/commit/96eb527fad0effe082b087c0898843feccc2e213))
>
> The new comment "Additionally, read-write operations performed outside a db txn context are disallowed by current BDB configurations" seems vague to me and I don't think would be comprehensible without reading this review
...

💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484771368)
Yeah, using `util::Interrupted` to skip the failure logging in the future sounds great. Error logging added.

💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484771531)
Done

🤔 sr-gi reviewed a pull request: "p2p: Don't process mutated blocks"
(https://github.com/bitcoin/bitcoin/pull/29412#pullrequestreview-1873071547)
Concept ACK

💬 sr-gi commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1484728486)
We should be able to simplify this as:

```cpp
if (!valid_opt.value_or(true)) return false;
```

💬 sr-gi commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1484773525)
It may be good to assert the debug log for the specific disconnection reason:

```suggestion
with self.nodes[0].assert_debug_log(expected_msgs=["bad-txnmrklroot, hashMerkleRoot mismatch"]):
attacker.send_message(msg_block(mutated_block))
```