💬 TheCharlatan commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1205612087)
This comment seems a bit inaccurate, e.g. we may have `stop_at_height` set, but shutdown is still triggered internally. There is also one error case in https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L931 that triggers a normal shutdown.
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1205612087)
This comment seems a bit inaccurate, e.g. we may have `stop_at_height` set, but shutdown is still triggered internally. There is also one error case in https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L931 that triggers a normal shutdown.
💬 pinheadmz commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1563088762)
> This PR is not modifying the rescan process. Only IBD.
sanity check: because we already skip old blocks during rescan?
https://github.com/bitcoin/bitcoin/pull/26679
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1563088762)
> This PR is not modifying the rescan process. Only IBD.
sanity check: because we already skip old blocks during rescan?
https://github.com/bitcoin/bitcoin/pull/26679
📝 hebasto opened a pull request: "Fix `#include`s in `src/wallet`"
(https://github.com/bitcoin/bitcoin/pull/27759)
This PR is a minimum required changes to fix https://github.com/bitcoin/bitcoin/pull/27571#discussion_r1195497290.
(https://github.com/bitcoin/bitcoin/pull/27759)
This PR is a minimum required changes to fix https://github.com/bitcoin/bitcoin/pull/27571#discussion_r1195497290.
💬 mzumsande commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1563115012)
> https://cirrus-ci.com/task/4751246913961984?logs=ci#L2724:
>
> ```shell
> test/denialofservice_tests.cpp(134): Entering test case "stale_tip_peer_management"
> test/denialofservice_tests.cpp(212): error: in "denialofservice_tests/stale_tip_peer_management": check vNodes[i]->fDisconnect == false has failed
> test/denialofservice_tests.cpp(214): error: in "denialofservice_tests/stale_tip_peer_management": check vNodes[max_outbound_full_relay - 2]->fDisconnect == true has failed
> test/den
...
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1563115012)
> https://cirrus-ci.com/task/4751246913961984?logs=ci#L2724:
>
> ```shell
> test/denialofservice_tests.cpp(134): Entering test case "stale_tip_peer_management"
> test/denialofservice_tests.cpp(212): error: in "denialofservice_tests/stale_tip_peer_management": check vNodes[i]->fDisconnect == false has failed
> test/denialofservice_tests.cpp(214): error: in "denialofservice_tests/stale_tip_peer_management": check vNodes[max_outbound_full_relay - 2]->fDisconnect == true has failed
> test/den
...
💬 MarcoFalke commented on pull request "log: don't log total disk read time in ConnectTip bench":
(https://github.com/bitcoin/bitcoin/pull/27673#issuecomment-1563172416)
lgtm ACK bc862fad294fdb3e4232734497d0693a0da4d63a 🐓
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK bc862fad294fdb3e
...
(https://github.com/bitcoin/bitcoin/pull/27673#issuecomment-1563172416)
lgtm ACK bc862fad294fdb3e4232734497d0693a0da4d63a 🐓
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK bc862fad294fdb3e
...
💬 D33r-Gee commented on issue "25.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/27736#issuecomment-1563183052)
OS: Ubuntu 20.04
Hello found an issue running the bonus test ["Test Command specified by shutdownnotify..."](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/25.0-Release-Candidate-Testing-Guide#test-command-specified-by-shutdownnotify-are-executed-synchronously-before-shutdown)
The following command didn’t work:
bitcoind-test -daemon -shutdownnotify="bcli touch hello.txt"
However after looking at the [#23395](https://github.com/bitcoin/bitcoin/pull/23395) PR, removing the bcli wor
...
(https://github.com/bitcoin/bitcoin/issues/27736#issuecomment-1563183052)
OS: Ubuntu 20.04
Hello found an issue running the bonus test ["Test Command specified by shutdownnotify..."](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/25.0-Release-Candidate-Testing-Guide#test-command-specified-by-shutdownnotify-are-executed-synchronously-before-shutdown)
The following command didn’t work:
bitcoind-test -daemon -shutdownnotify="bcli touch hello.txt"
However after looking at the [#23395](https://github.com/bitcoin/bitcoin/pull/23395) PR, removing the bcli wor
...
💬 joostjager commented on pull request "Allow accepting non-standard transactions on mainnet":
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1563206281)
I understand and appreciate the concerns about DoS protection and the importance of maintaining standardness rules for network-wide transaction relay. But isn't there room for flexibility when it comes to how Bitcoin nodes handle non-standard transactions submitted locally via the RPC methods sendrawtransaction and submitpackage?
These RPC calls are made by the node operator themselves or applications they trust, thus the risk of DoS attacks is significantly mitigated. The operator can assume
...
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1563206281)
I understand and appreciate the concerns about DoS protection and the importance of maintaining standardness rules for network-wide transaction relay. But isn't there room for flexibility when it comes to how Bitcoin nodes handle non-standard transactions submitted locally via the RPC methods sendrawtransaction and submitpackage?
These RPC calls are made by the node operator themselves or applications they trust, thus the risk of DoS attacks is significantly mitigated. The operator can assume
...
💬 MarcoFalke commented on issue "Spurious (?) valgrind failure for p2p_compactblocks.py":
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1563230842)
Looks like valgrind 3.21 still has this issue. I tried on Tumbleweed OpenSuse:
```
zypper in -y valgrind libevent-devel boost-devel find bison gcc-c++ libtool make autoconf automake python3 lbzip2 patch xz curl wget htop git vim ccache && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core && ./autogen.sh && ./configure --disable-tests --disable-bench && make -j $(nproc) && ./test/functional/p2p_compactblocks.py --valgrind
```
...
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1563230842)
Looks like valgrind 3.21 still has this issue. I tried on Tumbleweed OpenSuse:
```
zypper in -y valgrind libevent-devel boost-devel find bison gcc-c++ libtool make autoconf automake python3 lbzip2 patch xz curl wget htop git vim ccache && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core && ./autogen.sh && ./configure --disable-tests --disable-bench && make -j $(nproc) && ./test/functional/p2p_compactblocks.py --valgrind
```
...
💬 achow101 commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#issuecomment-1563232666)
ACK 89df7987c2f1eea42454c2b0efc31a924fbfd3a8
(https://github.com/bitcoin/bitcoin/pull/27145#issuecomment-1563232666)
ACK 89df7987c2f1eea42454c2b0efc31a924fbfd3a8
💬 darricksee commented on issue "Frequent "Timeout downloading block" with 24.1":
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1563336061)
I'd love to see the addition of `addr` too!
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1563336061)
I'd love to see the addition of `addr` too!
💬 achow101 commented on pull request "wallet, bench: Move commonly used functions to their own file and fix a bug":
(https://github.com/bitcoin/bitcoin/pull/27666#issuecomment-1563342926)
> Maybe instead of creating a new file, we could move the bench functions to `wallet/test/util.h` and `wallet/test/util.cpp` so they are used equally for tests and benchmarks?
Good point, made that change.
It required a few changes to the `CreateWallet` test since that wasn't expecting `postInitProcess` to be run, but should be straightforward to review.
(https://github.com/bitcoin/bitcoin/pull/27666#issuecomment-1563342926)
> Maybe instead of creating a new file, we could move the bench functions to `wallet/test/util.h` and `wallet/test/util.cpp` so they are used equally for tests and benchmarks?
Good point, made that change.
It required a few changes to the `CreateWallet` test since that wasn't expecting `postInitProcess` to be run, but should be straightforward to review.
💬 achow101 commented on pull request "wallet, bench: Move commonly used functions to their own file and fix a bug":
(https://github.com/bitcoin/bitcoin/pull/27666#discussion_r1205885446)
Dropped `wallet_common.h`, but still ended up doing this in `wallet_loading` and `wallet_balance`
(https://github.com/bitcoin/bitcoin/pull/27666#discussion_r1205885446)
Dropped `wallet_common.h`, but still ended up doing this in `wallet_loading` and `wallet_balance`
💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1563383911)
> sanity check: because we already skip old blocks during rescan?
>
> https://github.com/bitcoin/bitcoin/pull/26679
Yes. Different entry points.
#26679 addressed the scanning of the existing chain during load time.
And here, the emphasis shifts to the synchronization process when the
wallet is active.
When the node activate the best chain on each processed block and
signal the "block connected" event.
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1563383911)
> sanity check: because we already skip old blocks during rescan?
>
> https://github.com/bitcoin/bitcoin/pull/26679
Yes. Different entry points.
#26679 addressed the scanning of the existing chain during load time.
And here, the emphasis shifts to the synchronization process when the
wallet is active.
When the node activate the best chain on each processed block and
signal the "block connected" event.
📝 mzumsande opened a pull request: "p2p: Log addresses of stalling peers"
(https://github.com/bitcoin/bitcoin/pull/27761)
This was suggested in #27705 by ArmchairCryptologist.
It allows node operators that have the `-logips` option enabled to better identify potentially misbehaving peers and maybe ban them, which is especially helpful in case of inbound peers for which (dis)connections aren't logged per default, so it's impossible to use the debug log to connect their nodeId to an address unless the very noisy NET debugging is enabled.
In case of outbound peers for which the address is potentially logged when est
...
(https://github.com/bitcoin/bitcoin/pull/27761)
This was suggested in #27705 by ArmchairCryptologist.
It allows node operators that have the `-logips` option enabled to better identify potentially misbehaving peers and maybe ban them, which is especially helpful in case of inbound peers for which (dis)connections aren't logged per default, so it's impossible to use the debug log to connect their nodeId to an address unless the very noisy NET debugging is enabled.
In case of outbound peers for which the address is potentially logged when est
...
💬 brunoerg commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1563416721)
> Ah, I think the problem with the test is that AddRandomOutboundPeer creates random IPv4 addresses, and sometimes these aren't routable (e.g. protected ranges), so that they are not assigned to NET_IPV4 but NET_UNROUTABLE, which then makes the test fail. This happens intermittently. @amitiuttarwar I haven't looked into solutions yet, but may just try another IPv4 address if this is the case?
Not sure, I ran the test several times (debugging) and could get the error but didn't get a `NET_UNRO
...
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1563416721)
> Ah, I think the problem with the test is that AddRandomOutboundPeer creates random IPv4 addresses, and sometimes these aren't routable (e.g. protected ranges), so that they are not assigned to NET_IPV4 but NET_UNROUTABLE, which then makes the test fail. This happens intermittently. @amitiuttarwar I haven't looked into solutions yet, but may just try another IPv4 address if this is the case?
Not sure, I ran the test several times (debugging) and could get the error but didn't get a `NET_UNRO
...
💬 mzumsande commented on issue "Frequent "Timeout downloading block" with 24.1":
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1563417786)
> By all means, please do.
see #27761
> I expect reworking the block downloading logic would be fair from trivial
While that's certainly true, #27626 just made it into master and will help a lot with this! (not part of 25.0 though, it will need a bit more testing)
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1563417786)
> By all means, please do.
see #27761
> I expect reworking the block downloading logic would be fair from trivial
While that's certainly true, #27626 just made it into master and will help a lot with this! (not part of 25.0 though, it will need a bit more testing)
📝 willcl-ark opened a pull request: "contrib: refactor verify-binary print output"
(https://github.com/bitcoin/bitcoin/pull/27762)
Closes #27702
Adds functionality to the verify-binary.py script to verify the signatures in a torrent download.
Users must download the torrent themselves manually and point the script to the directory using the `torrent` subcommand.
The syntax chosen was `./contrib/verify-binaries/verify.py torrent <path to SHA256SUMS file>`
(https://github.com/bitcoin/bitcoin/pull/27762)
Closes #27702
Adds functionality to the verify-binary.py script to verify the signatures in a torrent download.
Users must download the torrent themselves manually and point the script to the directory using the `torrent` subcommand.
The syntax chosen was `./contrib/verify-binaries/verify.py torrent <path to SHA256SUMS file>`
💬 jamesob commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1563455744)
Rebased.
I've made a few changes/improvements that I think fixes the bug @sdaftuar found (not adding all potential tips to `setBlockIndexCandidates`), albeit I'm not yet integrating the changes in #27746.
- More comprehensive functional tests: the tests now stop and start at more points throughout the sync process, including immediately after snapshot load and before bg validation completes.
- We now include the blockhash of the snapshot base in `m_assumeutxo_data`; this allows us to move
...
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1563455744)
Rebased.
I've made a few changes/improvements that I think fixes the bug @sdaftuar found (not adding all potential tips to `setBlockIndexCandidates`), albeit I'm not yet integrating the changes in #27746.
- More comprehensive functional tests: the tests now stop and start at more points throughout the sync process, including immediately after snapshot load and before bg validation completes.
- We now include the blockhash of the snapshot base in `m_assumeutxo_data`; this allows us to move
...
💬 evansmj commented on issue "25.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/27736#issuecomment-1563469630)
Just documenting a discussion for a `bcli stop` command to be added at the end of -daemon sections. Mine was running in the background and was giving me failed to connect to any endpoint errors, calling `pkill bitcoind-test` resolves it but the experts in the chat think we should add to the guide calls to `bcli stop`.
stickies-v 12:02:47
evansmj: cc abubakarsadiq yeah i think adding a `bcli stop` command at the end of every section that started with running bitcoind in `-daemon` mode makes
...
(https://github.com/bitcoin/bitcoin/issues/27736#issuecomment-1563469630)
Just documenting a discussion for a `bcli stop` command to be added at the end of -daemon sections. Mine was running in the background and was giving me failed to connect to any endpoint errors, calling `pkill bitcoind-test` resolves it but the experts in the chat think we should add to the guide calls to `bcli stop`.
stickies-v 12:02:47
evansmj: cc abubakarsadiq yeah i think adding a `bcli stop` command at the end of every section that started with running bitcoind in `-daemon` mode makes
...
💬 evansmj commented on issue "25.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/27736#issuecomment-1563470556)
> OS: Ubuntu 20.04
>
> Hello found an issue running the bonus test ["Test Command specified by shutdownnotify..."](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/25.0-Release-Candidate-Testing-Guide#test-command-specified-by-shutdownnotify-are-executed-synchronously-before-shutdown)
>
> The following command didn’t work: bitcoind-test -daemon -shutdownnotify="bcli touch hello.txt"
>
> However after looking at the [#23395](https://github.com/bitcoin/bitcoin/pull/23395) PR, removing
...
(https://github.com/bitcoin/bitcoin/issues/27736#issuecomment-1563470556)
> OS: Ubuntu 20.04
>
> Hello found an issue running the bonus test ["Test Command specified by shutdownnotify..."](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/25.0-Release-Candidate-Testing-Guide#test-command-specified-by-shutdownnotify-are-executed-synchronously-before-shutdown)
>
> The following command didn’t work: bitcoind-test -daemon -shutdownnotify="bcli touch hello.txt"
>
> However after looking at the [#23395](https://github.com/bitcoin/bitcoin/pull/23395) PR, removing
...