✳️ achow101 pushed commits to a branch: bitcoin/bitcoin:master
(https://github.com/bitcoin/bitcoin/compare/fb2f0934799a...576e16e7026f)
Merge bitcoin/bitcoin#26184: test: p2p: check that headers message with invalid proof-of-work disconnects peer
772671245d50d94fd5087deb2542854604eba174 test: p2p: check that headers message with invalid proof-of-work disconnects peer (Sebastian Falbesoner)
Pull request description:
One of the earliest anti-DoS checks done after receiving and deserializing a `headers` message from a peer is verifying whether the proof-of-work is valid (called in method `PeerManagerImpl::ProcessHeadersMessage`):
https://github.com/bitcoin/bitcoin/blob/f227e153e80c8c50c30d76e1ac638d7206c7ff61/src/net_processing.cpp#L2752-L2762
The called method `PeerManagerImpl::CheckHeadersPoW` calls `Misbehaving` with a score of 100, i.e. leading to an immediate disconnect of the peer:
https://github.com/bitcoin/bitcoin/blob/f227e153e80c8c50c30d76e1ac638d7206c7ff61/src/net_processing.cpp#L2368-L2372
This PR adds a simple test for both the misbehaving log and the resulting disconnect. For creating a block header with invalid proof-of-work, we first create one that is accepted by the node (the difficulty field `nBits` is copied from the genesis block) and based on that the nonce is modified until we have block header hash prefix that is too high to fulfill even the minimum difficulty.
ACKs for top commit:
Sjors:
ACK 772671245d50d94fd5087deb2542854604eba174
achow101:
ACK 772671245d50d94fd5087deb2542854604eba174
brunoerg:
crACK 772671245d50d94fd5087deb2542854604eba174
furszy:
Code review ACK 77267124 with a non-blocking speedup.
Tree-SHA512: 680aa7939158d1dc672b90aa6554ba2b3a92584b6d3bcb0227776035858429feb8bc66eed18b47de0fe56df7d9b3ddaee231aaeaa360136603b9ad4b19e6ac11
(https://github.com/bitcoin/bitcoin/compare/fb2f0934799a...576e16e7026f)
Merge bitcoin/bitcoin#26184: test: p2p: check that headers message with invalid proof-of-work disconnects peer
772671245d50d94fd5087deb2542854604eba174 test: p2p: check that headers message with invalid proof-of-work disconnects peer (Sebastian Falbesoner)
Pull request description:
One of the earliest anti-DoS checks done after receiving and deserializing a `headers` message from a peer is verifying whether the proof-of-work is valid (called in method `PeerManagerImpl::ProcessHeadersMessage`):
https://github.com/bitcoin/bitcoin/blob/f227e153e80c8c50c30d76e1ac638d7206c7ff61/src/net_processing.cpp#L2752-L2762
The called method `PeerManagerImpl::CheckHeadersPoW` calls `Misbehaving` with a score of 100, i.e. leading to an immediate disconnect of the peer:
https://github.com/bitcoin/bitcoin/blob/f227e153e80c8c50c30d76e1ac638d7206c7ff61/src/net_processing.cpp#L2368-L2372
This PR adds a simple test for both the misbehaving log and the resulting disconnect. For creating a block header with invalid proof-of-work, we first create one that is accepted by the node (the difficulty field `nBits` is copied from the genesis block) and based on that the nonce is modified until we have block header hash prefix that is too high to fulfill even the minimum difficulty.
ACKs for top commit:
Sjors:
ACK 772671245d50d94fd5087deb2542854604eba174
achow101:
ACK 772671245d50d94fd5087deb2542854604eba174
brunoerg:
crACK 772671245d50d94fd5087deb2542854604eba174
furszy:
Code review ACK 77267124 with a non-blocking speedup.
Tree-SHA512: 680aa7939158d1dc672b90aa6554ba2b3a92584b6d3bcb0227776035858429feb8bc66eed18b47de0fe56df7d9b3ddaee231aaeaa360136603b9ad4b19e6ac11
🚀 achow101 merged a pull request: "test: p2p: check that headers message with invalid proof-of-work disconnects peer"
(https://github.com/bitcoin/bitcoin/pull/26184)
(https://github.com/bitcoin/bitcoin/pull/26184)
💬 sipa commented on pull request "Add simulation-based `CCoinsViewCache` fuzzer":
(https://github.com/bitcoin/bitcoin/pull/27011#issuecomment-1430564995)
> I saw that the fuzzer in coins_view.cpp still uses CCoinsMap coins_map;
I think that should probably be addressed.
> and doesn't use the deterministic seeds, shouldn't this fuzzer do that as well?
I believe it does?
```c++
CCoinsViewCache coins_view_cache{&backend_coins_view, /*deterministic=*/true};
```
> EDIT: I had a look what happens when I remove the default bool deterministic = false in SaltedOutpointHasher, and found another use of the map in the fuzzer, in tx_pool.
...
(https://github.com/bitcoin/bitcoin/pull/27011#issuecomment-1430564995)
> I saw that the fuzzer in coins_view.cpp still uses CCoinsMap coins_map;
I think that should probably be addressed.
> and doesn't use the deterministic seeds, shouldn't this fuzzer do that as well?
I believe it does?
```c++
CCoinsViewCache coins_view_cache{&backend_coins_view, /*deterministic=*/true};
```
> EDIT: I had a look what happens when I remove the default bool deterministic = false in SaltedOutpointHasher, and found another use of the map in the fuzzer, in tx_pool.
...
👍 ponury1990 approved a pull request: "wallet: SecureString to allow null characters"
(https://github.com/bitcoin/bitcoin/pull/27068)
(https://github.com/bitcoin/bitcoin/pull/27068)
💬 chinggg commented on pull request "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses":
(https://github.com/bitcoin/bitcoin/pull/26859#issuecomment-1430748624)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/26859#issuecomment-1430748624)
Concept ACK
💬 martinus commented on pull request "Add simulation-based `CCoinsViewCache` fuzzer":
(https://github.com/bitcoin/bitcoin/pull/27011#issuecomment-1430768763)
> I believe it does?
It does [in line 49 for `CCoinsViewCache`](https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/coins_view.cpp#L49), but not [in line 118 for the `CCoinsMap coins_map;`](https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/coins_view.cpp#L118). I've added that in #25325 [here](https://github.com/bitcoin/bitcoin/pull/25325/commits/78f597be2879c39d9d2b98e21ed0120d2308de20#diff-1ef3b6a1936b50f3d5ec4a1786d9e2d63d1a3e1815b103e67f20601995f355b4R119)
(https://github.com/bitcoin/bitcoin/pull/27011#issuecomment-1430768763)
> I believe it does?
It does [in line 49 for `CCoinsViewCache`](https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/coins_view.cpp#L49), but not [in line 118 for the `CCoinsMap coins_map;`](https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/coins_view.cpp#L118). I've added that in #25325 [here](https://github.com/bitcoin/bitcoin/pull/25325/commits/78f597be2879c39d9d2b98e21ed0120d2308de20#diff-1ef3b6a1936b50f3d5ec4a1786d9e2d63d1a3e1815b103e67f20601995f355b4R119)
🤔 LarryRuane requested changes to a pull request: "rpc, p2p: add `addpermissionflags` RPC and allow whitelisting outbound"
(https://github.com/bitcoin/bitcoin/pull/26441)
(https://github.com/bitcoin/bitcoin/pull/26441)
💬 LarryRuane commented on pull request "rpc, p2p: add `addpermissionflags` RPC and allow whitelisting outbound":
(https://github.com/bitcoin/bitcoin/pull/26441#discussion_r1106652331)
The first commit ("net: Move PF_ISIMPLICIT interpretation from AcceptConnection to ...") doesn't compile; this call to `AddWhitelistPermissionFlags()` should not have the `vWhitelistedRange` until the third commit, "Accept "in" and "out" flags to -whitelist...".
(https://github.com/bitcoin/bitcoin/pull/26441#discussion_r1106652331)
The first commit ("net: Move PF_ISIMPLICIT interpretation from AcceptConnection to ...") doesn't compile; this call to `AddWhitelistPermissionFlags()` should not have the `vWhitelistedRange` until the third commit, "Accept "in" and "out" flags to -whitelist...".
💬 LarryRuane commented on pull request "rpc, p2p: add `addpermissionflags` RPC and allow whitelisting outbound":
(https://github.com/bitcoin/bitcoin/pull/26441#discussion_r1106665680)
I got a compile error with the third commit, "Accept "in" and "out" flags to -whitelist..." because these are out of order, which you fixed later, but each commit should compile.
(https://github.com/bitcoin/bitcoin/pull/26441#discussion_r1106665680)
I got a compile error with the third commit, "Accept "in" and "out" flags to -whitelist..." because these are out of order, which you fixed later, but each commit should compile.
💬 MarcoFalke commented on issue "Stop the GPG verification madness":
(https://github.com/bitcoin/bitcoin/issues/25395#issuecomment-1430918927)
Not sure what is left to be done here in this thread. Lets continue discussion in the appropriate other threads:
* https://github.com/bitcoin-core/bitcoincore.org/issues/945 for the website
* https://github.com/bitcoin/bitcoin/issues/15774, https://github.com/bitcoin/bitcoin/issues/26176, https://github.com/bitcoin/bitcoin/issues/25834 for mac issues
* https://github.com/bitcoin/bitcoin/pull/23020 about the verify bin script
(https://github.com/bitcoin/bitcoin/issues/25395#issuecomment-1430918927)
Not sure what is left to be done here in this thread. Lets continue discussion in the appropriate other threads:
* https://github.com/bitcoin-core/bitcoincore.org/issues/945 for the website
* https://github.com/bitcoin/bitcoin/issues/15774, https://github.com/bitcoin/bitcoin/issues/26176, https://github.com/bitcoin/bitcoin/issues/25834 for mac issues
* https://github.com/bitcoin/bitcoin/pull/23020 about the verify bin script
✅ MarcoFalke closed an issue: "Stop the GPG verification madness"
(https://github.com/bitcoin/bitcoin/issues/25395)
(https://github.com/bitcoin/bitcoin/issues/25395)
✳️ MarcoFalke pushed commits to a branch: bitcoin/bitcoin:master
(https://github.com/bitcoin/bitcoin/compare/576e16e7026f...68e484afbbc2)
Merge bitcoin/bitcoin#26584: cli: include local ("unroutable") peers in -netinfo table
77192c959816dc8daee138d88bd6f3250ce3bdb6 cli: include local ("unreachable") peers in -netinfo table (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/26579
The `-netinfo` dashboard did not list peers that were connected via "unroutable" networks. This included local peers including local-network peers. Personally, I run one bitcoind instance on my network that is used by other services like Wasabi Wallet and LND running on other machines.
This PR adds an "npr" (not publicly routable) column to the table of networks (ipv4, ipv6, onion, etc) so that every connection to the node is listed, and the totals are accurate as they relate to max inbound and max outbound limits.
Example connecting in regtest mode to one local and one remote peer:
```
Bitcoin Core client v24.99.0-151ce099ea8f-dirty regtest - server 70016/Satoshi:24.99.0/
<-> type net mping ping send recv txn blk hb addrp addrl age id address version
in npr 0 0 90 90 1 1 127.0.0.1:59180 70016/Satoshi:24.99.0/
out manual ipv4 63 63 84 84 3 3 0 143.244.175.41 70016/Satoshi:24.0.1/
ms ms sec sec min min min
ipv4 ipv6 npr total block manual
in 0 0 1 1
out 1 0 0 1 0 1
total 1 0 1 2
Local addresses: n/a
```
ACKs for top commit:
jonatack:
Re-tested ACK 77192c959816dc8daee138d88bd6f3250ce3bdb6
Tree-SHA512: 78aa68bcff0dbaadb5f0604bf023fe8fd921313bd8276d12581f7655c089466a48765f9e123cb31d7f1d294d5ca45fdefdf8aa220466ff738f32414f41099c06
(https://github.com/bitcoin/bitcoin/compare/576e16e7026f...68e484afbbc2)
Merge bitcoin/bitcoin#26584: cli: include local ("unroutable") peers in -netinfo table
77192c959816dc8daee138d88bd6f3250ce3bdb6 cli: include local ("unreachable") peers in -netinfo table (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/26579
The `-netinfo` dashboard did not list peers that were connected via "unroutable" networks. This included local peers including local-network peers. Personally, I run one bitcoind instance on my network that is used by other services like Wasabi Wallet and LND running on other machines.
This PR adds an "npr" (not publicly routable) column to the table of networks (ipv4, ipv6, onion, etc) so that every connection to the node is listed, and the totals are accurate as they relate to max inbound and max outbound limits.
Example connecting in regtest mode to one local and one remote peer:
```
Bitcoin Core client v24.99.0-151ce099ea8f-dirty regtest - server 70016/Satoshi:24.99.0/
<-> type net mping ping send recv txn blk hb addrp addrl age id address version
in npr 0 0 90 90 1 1 127.0.0.1:59180 70016/Satoshi:24.99.0/
out manual ipv4 63 63 84 84 3 3 0 143.244.175.41 70016/Satoshi:24.0.1/
ms ms sec sec min min min
ipv4 ipv6 npr total block manual
in 0 0 1 1
out 1 0 0 1 0 1
total 1 0 1 2
Local addresses: n/a
```
ACKs for top commit:
jonatack:
Re-tested ACK 77192c959816dc8daee138d88bd6f3250ce3bdb6
Tree-SHA512: 78aa68bcff0dbaadb5f0604bf023fe8fd921313bd8276d12581f7655c089466a48765f9e123cb31d7f1d294d5ca45fdefdf8aa220466ff738f32414f41099c06
✅ MarcoFalke closed an issue: "bitcoin-cli -netinfo ignores inbound peer from local network"
(https://github.com/bitcoin/bitcoin/issues/26579)
(https://github.com/bitcoin/bitcoin/issues/26579)
🚀 MarcoFalke merged a pull request: "cli: include local ("unroutable") peers in -netinfo table"
(https://github.com/bitcoin/bitcoin/pull/26584)
(https://github.com/bitcoin/bitcoin/pull/26584)
💬 MarcoFalke commented on pull request "doc: Remove copyright years (headers only)":
(https://github.com/bitcoin/bitcoin/pull/26817#issuecomment-1430952811)
See also https://github.com/bitcoin/bitcoin/pull/27100
(https://github.com/bitcoin/bitcoin/pull/26817#issuecomment-1430952811)
See also https://github.com/bitcoin/bitcoin/pull/27100
💬 murrayn commented on pull request "doc: FreeBSD build doc should suggest db4 for legacy wallet support":
(https://github.com/bitcoin/bitcoin/pull/26773#issuecomment-1430998296)
I've updated the document to reflect the removal of `install_db4.sh` #26834
(https://github.com/bitcoin/bitcoin/pull/26773#issuecomment-1430998296)
I've updated the document to reflect the removal of `install_db4.sh` #26834
📝 ebarakos opened a pull request: "Fix minor typo"
(https://github.com/bitcoin/bitcoin/pull/27102)
<!--
Just a small typo fix
-->
(https://github.com/bitcoin/bitcoin/pull/27102)
<!--
Just a small typo fix
-->
💬 Sjors commented on pull request "Signing support for Miniscript Descriptors":
(https://github.com/bitcoin/bitcoin/pull/24149#issuecomment-1431108853)
@darosior do I understand correctly that you need #26567 in order for [PSBT stuff](https://github.com/bitcoin/bitcoin/pull/24149#issuecomment-1183382594) to work? Can I just combine both PR's to continue the test I was doing, or are there more changes required?
(https://github.com/bitcoin/bitcoin/pull/24149#issuecomment-1431108853)
@darosior do I understand correctly that you need #26567 in order for [PSBT stuff](https://github.com/bitcoin/bitcoin/pull/24149#issuecomment-1183382594) to work? Can I just combine both PR's to continue the test I was doing, or are there more changes required?
📝 fanquake converted_to_draft a pull request: "test: autogenerate bash completion"
(https://github.com/bitcoin/bitcoin/pull/25243)
Added a functional test which parses all the RPC help commands then automatically generates the bitcoin-cli bash-completion file and makes sure that the original file matches the newly generated one.
In order to get the RPC help commands in the correct format needed for the test, I added the "format" RPC command.
Test using `python3 test/functional/tool_cli_completion.py --overwrite`
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a ration
...
(https://github.com/bitcoin/bitcoin/pull/25243)
Added a functional test which parses all the RPC help commands then automatically generates the bitcoin-cli bash-completion file and makes sure that the original file matches the newly generated one.
In order to get the RPC help commands in the correct format needed for the test, I added the "format" RPC command.
Test using `python3 test/functional/tool_cli_completion.py --overwrite`
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a ration
...
💬 Sjors commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1431116683)
Concept ACK, I second @prusnak.
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1431116683)
Concept ACK, I second @prusnak.