💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1935973179)
I opened a new PR at https://github.com/bitcoin/bitcoin/pull/29415 to continue with this.

> in the current implementation, we schedule opening 5 connections per transaction, but then there's randomness around which one of the transactions each connection decides to broadcast. this could lead to a lot of variability if there are multiple transactions queued around the same time, which doesn't seem desirable. It makes more sense to me for there to be a direct mapping

@amitiuttarwar, I was th
...

✅ vasild closed a pull request: "Relay own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/27509)

💬 stratospher commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1484356402)
interesting script! sounds good to be on safer side.

nit: only if you have to retouch, you could update commit message in 100e8a75 to not mention `addmultisigaddress`

💬 stratospher commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#issuecomment-1935983955)
tested ACK 98570fe.

💬 1440000bytes commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-1936003083)
I am not sure about this approach to improve privacy. Is it necessary to open new short lived tor/i2p connections for broadcasting the transaction? What are the trade-offs in this implementation vs a simple implementation to relay tx to one or more peers that our node is already connected to?

Related issues:

https://github.com/bitcoin/bitcoin/issues/21876
https://github.com/bitcoin/bitcoin/issues/28636

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