💬 epiccurious commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1936008685)
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:49Z Writing 2529 me
...

🤔 sdaftuar reviewed a pull request: "v3 transaction policy for anti-pinning"
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1871529777)
Code review ACK. Left some comments that are just nits, nothing that I think needs to be addressed unless you have to fix more things anyway.

Will do another round of testing shortly...

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483769698)
nit: "replaceability"
(not worth fixing unless you need to touch this PR again)

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484331218)
Is dropping the locktime argument here intentional? (I guess all the explicit arguments are caught in kwargs?)

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483776227)
I can't figure out what this comment means? Maybe a holdover from when we were trimming 0-fee stuff?

💬 epiccurious commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1484386762)
very minor nit - To make the log more readable, can you please add a comma after the 'file'?

So it displays as:
```
Writing 2529 mempool transactions to file, xx MB...
```

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484387932)
I still would like a test for this, maybe as a follow-up

💬 epiccurious commented on pull request "fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse`":
(https://github.com/bitcoin/bitcoin/pull/29413#issuecomment-1936030123)
utACK 864e2e9097de8f1fda63137f803687dd5cc96c03.

💬 epiccurious commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#issuecomment-1936033190)
Concept ACK.

💬 epiccurious commented on pull request "test: Remove struct.pack from almost all places":
(https://github.com/bitcoin/bitcoin/pull/29401#issuecomment-1936039085)
Concept ACK. Is this ready for testing?

✅ jamesob closed a pull request: "Covenant tools softfork"
(https://github.com/bitcoin/bitcoin/pull/28550)

💬 maflcko commented on pull request "test: Remove struct.pack from almost all places":
(https://github.com/bitcoin/bitcoin/pull/29401#issuecomment-1936044118)
My preference would be to get the pull requests mentioned in the description merged first

💬 maflcko commented on pull request "test: Fix utxo set hash serialisation signedness":
(https://github.com/bitcoin/bitcoin/pull/29399#discussion_r1484406087)
Did that in https://github.com/bitcoin/bitcoin/pull/29401

💬 instagibbs commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#issuecomment-1936047945)
concept ACK

might be good to recap why it was only added in BLOCK processing but not other `ProcessBlocks`: In every other case we already don't punish the sender of compact blocks for failing these higher level checks, while full blocks allow for punishment.

💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484425597)
re: https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484322490

> In any case `m_substream{substream}` still creates a copy when it shouldn't, due to a missing `std::forward`, no?

Oh that's interesting. I wouldn't think to use `std::forward` because normally it's just used for && rvalue reference function template parameters, not class template parameters, and doesn't do anything useful if && is not used and special template deduction rules for it are not applied.

We are actual
...

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484427635)
nit: The reference to "in-package" doesn't fit here.

💬 epiccurious commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-1936072870)
Concept ACK 7dc0442ff5013827c051c8ff50c8fd8aa9440d7c.

💬 epiccurious commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#issuecomment-1936081626)
re-ACK 5b358cdd1a5f5d2fe87a9e41c638996eab2e2796.

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484436443)
> I could imagine this being useful if you wanted to write something like `ParamsStream mystream{FileStream{"file.bin"}, param1, param2}` and have the file automatically closed when the stream was destroyed.

Yes, an example would be in net.cpp, but feel free to ignore, if it is too much hassle.

```cpp
src/net.cpp- DataStream underlying_stream{vSeedsIn};
src/net.cpp: ParamsStream s{underlying_stream, CAddress::V2_NETWORK};

💬 epiccurious commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1936096250)
Concept ACK 0eec878e514c23d33d2cd81c44566ea601eb263f.