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

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484800862)
> If we get the same txid but different wtxid, I'm not sure that the result is necessarily one that makes sense. Most things in the wallet are keyed and listed by txid, so with this change, we'd end up with a transaction that's listed as conflicted by it's own txid. Also, we won't ever update the wallet to store a transaction with a different wtxid.

Sorry I didn't see this earlier. After looking at this again I don't think that same-txid-different-witness will be a problem here. Please see: h
...

💬 sr-gi commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1936624217)
ACK [13161ec](https://github.com/bitcoin/bitcoin/pull/27877/commits/13161ecf032b7a850686e5942c12222c8f3d0d52)

🚀 achow101 merged a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877)

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1936659009)
> looks like if I revert TX_MAX_STANDARD_VERSION to 3, all unit/functional tests pass?

I suppose my change at https://github.com/bitcoin/bitcoin/pull/28948/files#diff-cc0c6a9039a1c9fe38b8a21fe28391fffbac9b8531dfda0f658919a9f74b46baR793 is partially to blame. I made it `TX_MAX_STANDARD_VERSION + 1` instead of 3, thinking it might be easier to flip in the future.

I'm working on a followup to address the comments, and can incorporate if I need to retouch.

🤔 glozow reviewed a pull request: "p2p: Don't process mutated blocks"
(https://github.com/bitcoin/bitcoin/pull/29412#pullrequestreview-1873261879)
concept ACK, I agree with the approach of catching and dropping these earlier rather than later

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484847574)
Yes, seems this is because `SingleV3Checks` already covers it. I can add a comment and put the condition under an `Assume`?

💬 kevkevinpal commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1936684548)
> I'm seeing an issue during testing. The debug log incorrectly reports 0 MB. Are others able to reproduce my result?
>
> ```
> user1@comp1:~/Documents/GitHub/btc29402$ src/bitcoin-cli --rpcwait getmempoolinfo | jq -c '{ transactions: (.size), memory_usage_in_mb: (.usage/1000000) }' | jq; src/bitcoin-cli --rpcwait stop; sleep 10; grep -a Writing ~/.bitcoin/debug.log | tail -2
> {
> "transactions": 2529,
> "memory_usage_in_mb": 6.061648
> }
> Bitcoin Core stopping
> 2024-02-09T14:11
...