💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484235425)
> Maybe use string_view instead of string, since in most cases this is just passed a string literal, so no need to create a full string object

Sure!

💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484242585)
Sure!, done.

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

💬 theStack commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1484271021)
Good point. For legacy addresses on mainnet, it's very unlikely, but indeed theoretically possible to have a valid address that `IsHex`, i.e. consists of an even number of hex characters. One example for this is 111111111111111111126C31DfD9, found with the following Python script using our test framework:
```python
#!/usr/bin/env python3
from test_framework.address import keyhash_to_p2pkh

for i in range(100000):
addr = keyhash_to_p2pkh(i.to_bytes(20, 'big'), main=True)
try:

...

💬 cornwarecjp commented on issue "Incorrect amount in transaction page":
(https://github.com/bitcoin/bitcoin/issues/29378#issuecomment-1935867840)
ACK, it's fixed: in version 26, the transaction shows as +0.0009 BTC, and the total of the transaction amounts equals the wallet balance.

Note to future readers of this issue: when you add up the amounts in the CSV file, don't forget to skip the amounts of the non-confirmed transactions (e.g. ones that have been RBFed).

👍 vasild approved a pull request: "fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse`"
(https://github.com/bitcoin/bitcoin/pull/29413#pullrequestreview-1872299532)
ACK 864e2e9097de8f1fda63137f803687dd5cc96c03

💬 brunoerg commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1935886953)
Force-pushed addressing:
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1480525652
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1480624819
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1481727947
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483435534

Thanks @sr-gi for reviewing!

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

> [3c31173](https://github.com/bitcoin/bitcoin/commit/3c311734d2fc6a4ca410f254ba3c8e923d58be70):
>
> I don't understand this. For example, if `SubStream` is a `DataStream` that holds data, this will now create a copy of this data, when previously it didn't?

I will add a comment here, but this does not happen due to the "Template deduction guide for a single params argument that's slightly different from the default
...

💬 jadijadi commented on issue "Weird focus rect displayed on inital sync":
(https://github.com/bitcoin-core/gui/issues/783#issuecomment-1935930424)
Tried to reproduce this on 26.9.9 with no successful.
May I ask how you produced the "expected result" image? what do you do to "fix" the issue? And is there a way to "show/hide" the strange rectangular? I can see that in the problematic photo you have the Hide button focused too. Does losing the focus hides the strange highlight square?

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(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?

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

📝 vasild opened a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415)
To improve privacy, broadcast locally submitted transactions (from the `sendrawtransaction` RPC) to the P2P network only via Tor or I2P short-lived connections.

* Introduce a new connection type for private broadcast of transactions with the following properties:
* started whenever there are local transactions to be sent
* only opened to Tor or I2P peers
* opened regardless of max connections limits
* after handshake is completed one local transaction is pushed to the peer, `PING`
...

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