💬 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))
```

💬 sr-gi commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1484725603)
Same as for `ContextualCheckBlock`, this can be simplified to:

```cpp
if (valid_opt.value_or(true)) {
```

💬 sr-gi commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1484744076)
Basically performing `CheckBlockHeader` here too?

That sounds reasonable to me, and the outcome is the same IIRC: `Misbehaving(*peer, 100, ...`

Also, this is cheap enough that it may not matter if we do it again later (?), so we may not even need to cache it (correct me if I'm wrong)

🤔 furszy reviewed a pull request: "wallet: batch erase procedures and improve 'EraseRecords' performance"
(https://github.com/bitcoin/bitcoin/pull/29403#pullrequestreview-1873150521)
Updated per feedback. Thanks ryanofsky!. [Small diff](https://github.com/bitcoin/bitcoin/compare/8330145ba88b39ddaa9e0bbfc2d1c198da652249..2095ccdca0cf09073b51b3e9e0d5b9931825824d).
Per request, now `RunWithinTxn` logs an error when the provided function requires to abort the db txn. As well as two comments have been improved for clarity.

💬 1440000bytes commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-1936576857)
I think I forgot mentioned:

- You have been working since last year
- Lot of years reviewed

Still I would NACK if it does not work for BITCOIN

💬 instagibbs commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1484789046)
Ok this doesn't seem to matter: we do this hashing even for already-known blocks we get sent unilaterally, and `MAX_PROTOCOL_MESSAGE_LENGTH` bounds the size of hashing to something reasonable.

:lock: fanquake locked an issue: "Enable sanctions enforcement in Spam Filter Code"
(https://github.com/bitcoin/bitcoin/issues/29416)

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484797295)
> Would be good to explain that you are doing the mapWallet lookup because the wtx could have been updated.

I just realized that the wtx could not have been updated, because for it to be updated, `transactionAddedToMempool` would need to be called, which would be after this call, right? So I have reverted the change which compares witness hashes.

> Also, as you already know that the output belongs to one of the wallet's txs, could:

Yes, I have implemented this.