💬 vasild commented on issue "new RPC: sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/issues/28636#issuecomment-1937005765)
Notice that rebroadcast in this way wouldn't achieve anything if the recipient has the same connections to the network. This is because upon the first receipt of the transaction the peer will announce it to all peers they are connected to and will remember that they have been told about the transaction. When the re-broadcast comes the peer will not announce the transaction a second time to the same peers.
The broadcast can be done in such a way that you don't have to trust the peer (privacy w
...
(https://github.com/bitcoin/bitcoin/issues/28636#issuecomment-1937005765)
Notice that rebroadcast in this way wouldn't achieve anything if the recipient has the same connections to the network. This is because upon the first receipt of the transaction the peer will announce it to all peers they are connected to and will remember that they have been told about the transaction. When the re-broadcast comes the peer will not announce the transaction a second time to the same peers.
The broadcast can be done in such a way that you don't have to trust the peer (privacy w
...
💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1937043749)
> > Should I PR the above branch?
> Hey Vasil, I think this makes most sense ...
I am on it!
> Your comments https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1415682660 did get me thinking whether we could use mapRecvBytesPerMsgType to return the netmsgstats, or conversely use data from net_stats to return stats to the getpeerinfo RPC and avoid counting the same message twice everywhere. But I'm not convinced that the changes that would be needed to do that would be worth it.
...
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1937043749)
> > Should I PR the above branch?
> Hey Vasil, I think this makes most sense ...
I am on it!
> Your comments https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1415682660 did get me thinking whether we could use mapRecvBytesPerMsgType to return the netmsgstats, or conversely use data from net_stats to return stats to the getpeerinfo RPC and avoid counting the same message twice everywhere. But I'm not convinced that the changes that would be needed to do that would be worth it.
...
💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1937045521)
> ... this requires the "inverse" ...
Ha, I did not realize this before! I am fine either way. Also I am not sure what would be the best name of the RPC parameter, e.g. `bitcoin-cli -named getnetmsgstats whatname='[...]'`. Lets do a poll below (use :+1:).
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1937045521)
> ... this requires the "inverse" ...
Ha, I did not realize this before! I am fine either way. Also I am not sure what would be the best name of the RPC parameter, e.g. `bitcoin-cli -named getnetmsgstats whatname='[...]'`. Lets do a poll below (use :+1:).
💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1937045585)
To produce the output in https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1908444454 it is better to use `bitcoin-cli getnetmsgstats '["message_type"]'`. How to name the RPC parameter?
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1937045585)
To produce the output in https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1908444454 it is better to use `bitcoin-cli getnetmsgstats '["message_type"]'`. How to name the RPC parameter?
💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1937045606)
To produce the output in https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1908444454 it is better to use `bitcoin-cli getnetmsgstats '["direction", "network", "connection_type"]'`. How to name the RPC parameter?
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1937045606)
To produce the output in https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1908444454 it is better to use `bitcoin-cli getnetmsgstats '["direction", "network", "connection_type"]'`. How to name the RPC parameter?
💬 YellowRoseCx commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1937099385)
> I updated the numbers and rebased the pull-req.
>
> We should get this merged for the next release. It's pretty silly to have a default mempool policy that only 30% of hash power is following.
Over 80% of the network's hashpower was once signalling for SegWit2x, but that much hashrate approval didn't matter regarding implementations of changes then. Plus, a default Replace-By-Fee will just make BTC harder to use as a transactional currency without having to go through a medium, "and ther
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1937099385)
> I updated the numbers and rebased the pull-req.
>
> We should get this merged for the next release. It's pretty silly to have a default mempool policy that only 30% of hash power is following.
Over 80% of the network's hashpower was once signalling for SegWit2x, but that much hashrate approval didn't matter regarding implementations of changes then. Plus, a default Replace-By-Fee will just make BTC harder to use as a transactional currency without having to go through a medium, "and ther
...
💬 kristapsk commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1937100741)
> Over 80% of the network's hashpower was once signalling for SegWit2x, but that much hashrate approval didn't matter
Segwit2x was change in consensus rules, a hardfork, full-RBF is not, it's mempool policy, which isn't part of consensus.
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1937100741)
> Over 80% of the network's hashpower was once signalling for SegWit2x, but that much hashrate approval didn't matter
Segwit2x was change in consensus rules, a hardfork, full-RBF is not, it's mempool policy, which isn't part of consensus.
💬 mzumsande commented on issue "Update security.md with all PGP fingerprints":
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1937108212)
Also, the linked page refers only to the special case of embargoed hardware issues. For the topic of software security bugs in the linux kernel (which seems like a better comparison), the page https://www.kernel.org/doc/html/v6.8-rc1/process/security-bugs.html applies, which points to a "private list of security officers" where no member is explicitly named.
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1937108212)
Also, the linked page refers only to the special case of embargoed hardware issues. For the topic of software security bugs in the linux kernel (which seems like a better comparison), the page https://www.kernel.org/doc/html/v6.8-rc1/process/security-bugs.html applies, which points to a "private list of security officers" where no member is explicitly named.
💬 epiccurious commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1937116907)
Tested ACK 25482b5aea6fb910b74666414a6330ec53f6a4c9.
This output is clean. Solid approach piggy-backing on the `Dumped mempool: ` line that already exists.
```
2024-02-10T20:36:10Z Writing 5364 mempool transactions to file...
2024-02-10T20:36:10Z Writing 0 unbroadcast transactions to file.
2024-02-10T20:36:10Z Dumped mempool: 0.005s to copy, 0.101s to dump, 7.865 MB dumped to file
```
> they matched in sizes
ACK.
```
user1@comp1:~$ ls -l .bitcoin/mempool.dat
-rw------- 1 user1
...
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1937116907)
Tested ACK 25482b5aea6fb910b74666414a6330ec53f6a4c9.
This output is clean. Solid approach piggy-backing on the `Dumped mempool: ` line that already exists.
```
2024-02-10T20:36:10Z Writing 5364 mempool transactions to file...
2024-02-10T20:36:10Z Writing 0 unbroadcast transactions to file.
2024-02-10T20:36:10Z Dumped mempool: 0.005s to copy, 0.101s to dump, 7.865 MB dumped to file
```
> they matched in sizes
ACK.
```
user1@comp1:~$ ls -l .bitcoin/mempool.dat
-rw------- 1 user1
...
💬 davidgumberg commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1485310634)
While touching this, maybe `BCLOG::UTIL` which is not used since: https://github.com/bitcoin/bitcoin/pull/27896 should be dropped?
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1485310634)
While touching this, maybe `BCLOG::UTIL` which is not used since: https://github.com/bitcoin/bitcoin/pull/27896 should be dropped?
💬 epiccurious commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-1937228496)
v2 Transport will be enabled by default in the next release (https://github.com/bitcoin/bitcoin/pull/29347).
If there were eventually a change to _force_ clearnet transactions over v2 transport (so the details of the communications were encrypted), would that solve the same problem that this PR is aiming to solve?
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-1937228496)
v2 Transport will be enabled by default in the next release (https://github.com/bitcoin/bitcoin/pull/29347).
If there were eventually a change to _force_ clearnet transactions over v2 transport (so the details of the communications were encrypted), would that solve the same problem that this PR is aiming to solve?
💬 ishaanam commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1485316816)
Done
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1485316816)
Done
💬 ishaanam commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1485317023)
Done
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1485317023)
Done
💬 ishaanam commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1485317291)
Done, but it is 34 because the wallet also received 17 previously.
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1485317291)
Done, but it is 34 because the wallet also received 17 previously.
💬 ishaanam commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1485317498)
Done
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1485317498)
Done
💬 epiccurious commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1937358474)
Concept ACK 70171e012a7cd6723aa2b07a3fb3d93920ec2010.
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1937358474)
Concept ACK 70171e012a7cd6723aa2b07a3fb3d93920ec2010.
🤔 tdb3 reviewed a pull request: "test: adds outbound eviction functional tests, updates comment in ConsiderEviction"
(https://github.com/bitcoin/bitcoin/pull/29122#pullrequestreview-1874277464)
Excellent work finding a gap and filling it by increasing functional test robustness.
ACK for edc14bc9da332cae39f8803db559b532b3c74e16
Checked out the PR branch, built, and ran all functional tests (all passed).
The review was focused on verifying case coverage of the tests and that tests will accurately catch both pass and fail conditions.
Did not observe previously reported warning message:
```
TestFramework.p2p (WARNING): Connection lost to 0:0 due to [Errno 54] Connection reset b
...
(https://github.com/bitcoin/bitcoin/pull/29122#pullrequestreview-1874277464)
Excellent work finding a gap and filling it by increasing functional test robustness.
ACK for edc14bc9da332cae39f8803db559b532b3c74e16
Checked out the PR branch, built, and ran all functional tests (all passed).
The review was focused on verifying case coverage of the tests and that tests will accurately catch both pass and fail conditions.
Did not observe previously reported warning message:
```
TestFramework.p2p (WARNING): Connection lost to 0:0 due to [Errno 54] Connection reset b
...
💬 tdb3 commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1485367696)
I could be missing something, but a test case covering the reset of eviction timeout when an outbound peer sends us a block with a valid longer chainwork (i.e. the timeout set to 0 to prevent eviction, and m_work_header/m_sent_getheaders also reinitialized) was not observed. Do you think it's worth adding?
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1485367696)
I could be missing something, but a test case covering the reset of eviction timeout when an outbound peer sends us a block with a valid longer chainwork (i.e. the timeout set to 0 to prevent eviction, and m_work_header/m_sent_getheaders also reinitialized) was not observed. Do you think it's worth adding?
💬 tdb3 commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1485370601)
Added notes below to provide insight into what was reviewed:
If eviction does not occur when it should, then `wait_for_disconnect()` will raise (AssertionError, originally from `wait_until_helper_internal()`). Confirmed by temporarily setting `cur_mock_time` to a value significantly lower than CHAIN_SYNC_TIMEOUT.
Similarly, if unexpected eviction occurs (e.g. in the "keep catching up" case), then `send_and_ping()` or `sync_with_ping()` will raise IOError (`from send_raw_message()`). Conf
...
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1485370601)
Added notes below to provide insight into what was reviewed:
If eviction does not occur when it should, then `wait_for_disconnect()` will raise (AssertionError, originally from `wait_until_helper_internal()`). Confirmed by temporarily setting `cur_mock_time` to a value significantly lower than CHAIN_SYNC_TIMEOUT.
Similarly, if unexpected eviction occurs (e.g. in the "keep catching up" case), then `send_and_ping()` or `sync_with_ping()` will raise IOError (`from send_raw_message()`). Conf
...
💬 tdb3 commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1485375517)
nit: I could be missing something, but a test case covering the limit of protected outbound peers (e.g. `MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT`) was not observed. Do you think it's worth adding?
Thinking out loud:
`MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT = 4`, so maybe adding another peer that would normally be considered protected (i.e. one that provided a block with the same amount of work as the tip) and seeing that it is not protected could test this?
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1485375517)
nit: I could be missing something, but a test case covering the limit of protected outbound peers (e.g. `MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT`) was not observed. Do you think it's worth adding?
Thinking out loud:
`MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT = 4`, so maybe adding another peer that would normally be considered protected (i.e. one that provided a block with the same amount of work as the tip) and seeing that it is not protected could test this?