💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1936918798)
Rebased 6752d218caeed1111f2520130c156b9ef42ba805 -> ed3ca5831515c26cf4242de9d13dbe0eb82f6d03 ([noGlobalSignals_13](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_13) -> [noGlobalSignals_14](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_14), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_13..noGlobalSignals_14))
* Fixed conflict with https://github.com/bitcoin/bitcoin/pull/28948
Updated ed3ca5831515c26cf4242de9d13dbe0eb82f6d03 -> c02fd
...
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1936918798)
Rebased 6752d218caeed1111f2520130c156b9ef42ba805 -> ed3ca5831515c26cf4242de9d13dbe0eb82f6d03 ([noGlobalSignals_13](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_13) -> [noGlobalSignals_14](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_14), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_13..noGlobalSignals_14))
* Fixed conflict with https://github.com/bitcoin/bitcoin/pull/28948
Updated ed3ca5831515c26cf4242de9d13dbe0eb82f6d03 -> c02fd
...
💬 BrandonOdiwuor commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-1936935414)
@maflcko I have added a basic test for the `abortrescan` RPC and `TODO` for adding a test when `rescanblockchain` is running
I cannot add the second test since I'm unable to get `abortrescan` to execute when a rescan is ongoing
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-1936935414)
@maflcko I have added a basic test for the `abortrescan` RPC and `TODO` for adding a test when `rescanblockchain` is running
I cannot add the second test since I'm unable to get `abortrescan` to execute when a rescan is ongoing
💬 TheCharlatan commented on pull request "refactor: Replace sets of txiter with CTxMemPoolEntryRefs":
(https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1936945949)
Rebased 54f0dc5adc17ca976d9c6e997f317dc22318768f -> c34dcea019ea3b639b9f387df18777bc9a2d1534 ([setEntryRefs_3](https://github.com/TheCharlatan/bitcoin/tree/setEntryRefs_3) -> [setEntryRefs_4](https://github.com/TheCharlatan/bitcoin/tree/setEntryRefs_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/setEntryRefs_3..setEntryRefs_4))
* Fxied conflict with https://github.com/bitcoin/bitcoin/pull/28948
(https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1936945949)
Rebased 54f0dc5adc17ca976d9c6e997f317dc22318768f -> c34dcea019ea3b639b9f387df18777bc9a2d1534 ([setEntryRefs_3](https://github.com/TheCharlatan/bitcoin/tree/setEntryRefs_3) -> [setEntryRefs_4](https://github.com/TheCharlatan/bitcoin/tree/setEntryRefs_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/setEntryRefs_3..setEntryRefs_4))
* Fxied conflict with https://github.com/bitcoin/bitcoin/pull/28948
💬 stickies-v commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1485037848)
The main point of b2fb55cabc76d842c58b51ff9c64126e6639d1bb is to remove the `testmempoolaccept` reference. I use the opportunity to align the string notation with the (I think) more recent usage of raw literal string notation, which is a bit clearer imo. Using quotation marks around the "variable" name seems consistent with every other instance I've found, e.g. [`testmempoolaccept`](https://github.com/bitcoin/bitcoin/blob/7143d4388407ab3d12005e55a02d5e8f334e4dc9/src/rpc/mempool.cpp#L153), [`gene
...
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1485037848)
The main point of b2fb55cabc76d842c58b51ff9c64126e6639d1bb is to remove the `testmempoolaccept` reference. I use the opportunity to align the string notation with the (I think) more recent usage of raw literal string notation, which is a bit clearer imo. Using quotation marks around the "variable" name seems consistent with every other instance I've found, e.g. [`testmempoolaccept`](https://github.com/bitcoin/bitcoin/blob/7143d4388407ab3d12005e55a02d5e8f334e4dc9/src/rpc/mempool.cpp#L153), [`gene
...
💬 stickies-v commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1485039911)
I've thought about that approach but decided not to because:
- even though it's a helpful message, the user trying to submit a package of size 1 almost by definition means they don't understand the package topology requirements. Therefore, I think pointing that out is more helpful so the user can read up on the documentation right away. (of course, we could provide more specific error messages during package topology validation, but that's orthogonal to this PR)
- we'd be performing the same v
...
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1485039911)
I've thought about that approach but decided not to because:
- even though it's a helpful message, the user trying to submit a package of size 1 almost by definition means they don't understand the package topology requirements. Therefore, I think pointing that out is more helpful so the user can read up on the documentation right away. (of course, we could provide more specific error messages during package topology validation, but that's orthogonal to this PR)
- we'd be performing the same v
...
💬 stickies-v commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#issuecomment-1936958897)
Rebased to master to hopefully make CI green, no changes otherwise. Thanks for the review @glozow, addressed all comments.
(https://github.com/bitcoin/bitcoin/pull/29292#issuecomment-1936958897)
Rebased to master to hopefully make CI green, no changes otherwise. Thanks for the review @glozow, addressed all comments.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-1936992757)
@1440000bytes, thanks for asking! There is some discussion at https://github.com/bitcoin/bitcoin/pull/27509 (the previous attempt on this).
> Is it necessary to open new short lived tor/i2p connections for broadcasting the transaction?
Yes, it is. See below.
> 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?
Sending the transaction over clearnet reveals the IP address/geolocation of the s
...
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-1936992757)
@1440000bytes, thanks for asking! There is some discussion at https://github.com/bitcoin/bitcoin/pull/27509 (the previous attempt on this).
> Is it necessary to open new short lived tor/i2p connections for broadcasting the transaction?
Yes, it is. See below.
> 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?
Sending the transaction over clearnet reveals the IP address/geolocation of the s
...
💬 glozow commented on issue "Update security.md with all PGP fingerprints":
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1936996296)
I agree this can be closed as not planned. You can encrypt your communications to a specific key. The linked page seems to have the same practice we do. There seems to be an accusation that the security list is untrustworthy, leading to demands that we do things that are nonstandard, put people at risk for no reason, and ultimately unhelpful to handling security issues.
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1936996296)
I agree this can be closed as not planned. You can encrypt your communications to a specific key. The linked page seems to have the same practice we do. There seems to be an accusation that the security list is untrustworthy, leading to demands that we do things that are nonstandard, put people at risk for no reason, and ultimately unhelpful to handling security issues.
💬 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