💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1662136231)
`2541f09439...6e6f83b0f7`: rebase due to conflicts and address some suggestions and issues. Also ditch `UseSensitiveRelay()` and use `m_opts.sensitiverelayowntx` directly.

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1281846678)
Done

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1281847092)
Done

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1281847815)
Moved, thanks!

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1281851228)
Good! This should be moved a bit below, after the transaction has been added to the mempool: `const MempoolAcceptResult result = m_chainman.ProcessTransaction(ptx);` and conditional on `result`.

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1281854368)
Yes. This is the case for all connections - if the peer goes silent, but does not close the connection, then we will close it after 20 minutes. Do you think that is not suitable for sensitive relay connections? If yes, what to do better?

💬 brunoerg commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#issuecomment-1662147529)
Concept ACK

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1281863833)
For sensitive relay connections it is not possible to fingerprint based on the version handshake or any other messages exchanged (see the OP under "The messages exchange should look like this").

The only non-constant in the `VERSION` message we send is the peer nonce, then `VERACK` has no payload and the only payload in the `PING` is the ping nonce.

💬 dergoegge commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1281864257)
Oh for some reason I thought our ping timeout was smaller than `TIMEOUT_INTERVAL` (turns out it's the same). Feel free to consider this a nit.

But I think it would be reasonable for these connections to timeout much quicker anyway, since we don't care about keeping them around.

💬 dergoegge commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1281871273)
> The only non-constant in the VERSION message we send is the peer nonce

Yes and this can be used for fingerprinting, because we disconnect incoming connections that match an already existing nonce.

https://github.com/bitcoin/bitcoin/blob/2fa60f0b683cefd7956273986dafe3bde00c98fd/src/net_processing.cpp#L3361-L3366

And I wouldn't be surprised if there are more ways to fingerprint in the version handshake. This will also end up being a maintenance concern going forward, mostly as a burden
...

💬 jonatack commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#issuecomment-1662205614)
Concept ACK

💬 brunoerg commented on pull request "rpc, test: `addnode` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281911771)
Done!

💬 brunoerg commented on pull request "rpc, test: `addnode` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281911898)
Done!

💬 brunoerg commented on pull request "rpc, test: `addnode` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281912594)
Done

💬 brunoerg commented on pull request "rpc, test: `addnode` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#issuecomment-1662221927)
Force-pushed addressing @jonatack's review. Addressed:

- https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281091853
- https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281087542
- https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281083516

🤔 jonatack reviewed a pull request: "rpc, test: `addnode` improv + add test coverage for invalid command"
(https://github.com/bitcoin/bitcoin/pull/26366#pullrequestreview-1559045632)
ACK f52cb02f700b58bca921a7aa24bfeee04760262b

Two non-blocking suggestions.

💬 jonatack commented on pull request "rpc, test: `addnode` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281931598)
<details><summary>suggestion while touching this</summary><p>

```diff
- if (command == "onetry")
- {
+ if (command == "onetry") {
CAddress addr;
connman.OpenNetworkConnection(addr, /*fCountFailure=*/false, /*grantOutbound=*/nullptr, node_arg.c_str(), ConnectionType::MANUAL);
return UniValue::VNULL;
- }
-
- if (command == "add")
- {
+ } elsif (command == "add") {
if (!connman.AddNode(node_arg)) {
throw JSONRPCEr
...

💬 jonatack commented on pull request "rpc, test: `addnode` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281927417)
"remove the check for nullance for command since it's a non-optional field"

while here, maybe test that assertion

```diff
- # check that an invalid command returns an error
+ # check that an invalid or missing command returns an error
assert_raises_rpc_error(-1, 'addnode "node" "command"', self.nodes[0].addnode, node=ip_port, command='abc')
+ assert_raises_rpc_error(-1, 'addnode "node" "command"', self.nodes[0].addnode, node=ip_port)
```

💬 MarcoFalke commented on issue "build: Windows debug cross-build fails with `-O0`":
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1662256481)
Looking at `-ftime-trace=/tmp/`, I don't think there is anything that can be done, other than to nuke boost, or to define a smaller limited interface of our boost usage in a new header and hide the boost includes in the corresponding cpp file?


💬 willcl-ark commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1662256836)
Concept ACK.

I've not reviewed the code in detail yet, but this is a nice followup to #28060.

It seems to work well so far. I wrote a quick-n-dirty [python script](https://gist.github.com/willcl-ark/d1dd7d80d4581671aa38157960a87ba7) to manually xor my data dir using the same 8 byte key from this pull, and Bitcoin Core at commit fa3c1d230a didn't have any problems reading the xored blocks or key afterwards.

💬 hebasto commented on pull request "[WIP] guix: use `-muse-unaligned-vector-move` for Windows builds":
(https://github.com/bitcoin/bitcoin/pull/28151#issuecomment-1662299643)
Tested Guix build on Windows:

```
C:\Users\hebasto\Desktop\pr28151\bitcoin-6c3f6a9d015c-win64\bitcoin-6c3f6a9d015c>bin\bitcoind.exe -signet
*** stack smashing detected ***: terminated

```