💬 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
...
💬 evansmj commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1563521122)
- MacOS 13.3.1
- 25.0rc2 compiled from source
- tested through the entire guide, including bonus and verifying binaries test.
- everything worked, documented a test guide comment in the test guide [issue](https://github.com/bitcoin/bitcoin/issues/27736) as we discussed in irc.
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1563521122)
- MacOS 13.3.1
- 25.0rc2 compiled from source
- tested through the entire guide, including bonus and verifying binaries test.
- everything worked, documented a test guide comment in the test guide [issue](https://github.com/bitcoin/bitcoin/issues/27736) as we discussed in irc.
💬 TheCharlatan commented on pull request "kernel: Remove util/system from kernel library, interface_ui from validation.":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1206033293)
Will add this to the TODO section in the tracking issue.
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1206033293)
Will add this to the TODO section in the tracking issue.
💬 theuni commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1563561502)
I spent some time reviving my clang-tidy transform plugin which is able to force bubble-ups of fatal errors and decorations of functions which may call them. I'm not suggesting that we use this approach (I mentioned at core-dev that it works but I find it pretty ugly), though it's useful to point out our current shutdown behavior.
Note that the plugin is self-written, so it's not flawless. It does produce compiling code though, so I have a reasonable amount of confidence.
[See here for the
...
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1563561502)
I spent some time reviving my clang-tidy transform plugin which is able to force bubble-ups of fatal errors and decorations of functions which may call them. I'm not suggesting that we use this approach (I mentioned at core-dev that it works but I find it pretty ugly), though it's useful to point out our current shutdown behavior.
Note that the plugin is self-written, so it's not flawless. It does produce compiling code though, so I have a reasonable amount of confidence.
[See here for the
...
💬 sdaftuar commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1563571639)
> All blocks for which we have data (or are assumedvalid) are added to all chainstates' setBlockIndexCandidates.
Being assumedvalid is unrelated to whether we should add an entry to `setBlockIndexCandidates`; `setBlockIndexCandidates` should have the current tip and any block for which we HAVE_DATA and for which we HAVE_DATA for all the blocks that we'd need to connect, in order to get from our current tip to that target block.
The assumedvalid bit is really just something we're using to
...
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1563571639)
> All blocks for which we have data (or are assumedvalid) are added to all chainstates' setBlockIndexCandidates.
Being assumedvalid is unrelated to whether we should add an entry to `setBlockIndexCandidates`; `setBlockIndexCandidates` should have the current tip and any block for which we HAVE_DATA and for which we HAVE_DATA for all the blocks that we'd need to connect, in order to get from our current tip to that target block.
The assumedvalid bit is really just something we're using to
...
💬 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-1563619750)
To review this PR I wrote a simple functional test: https://github.com/pinheadmz/bitcoin/commit/1f0f945aca3a4f5118e49b2d31318ddd4b19d906 which surprised me by failing. I added logging and comments to the test to share it because I don't entirely understand what's happening but it looks to me like, the wallet generates a `combo(...)` descriptor with a timestamp of `1` and therefore the wallet creation time is always `1` and so the new logic doesn't actually skip any blocks. But if that's the case
...
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1563619750)
To review this PR I wrote a simple functional test: https://github.com/pinheadmz/bitcoin/commit/1f0f945aca3a4f5118e49b2d31318ddd4b19d906 which surprised me by failing. I added logging and comments to the test to share it because I don't entirely understand what's happening but it looks to me like, the wallet generates a `combo(...)` descriptor with a timestamp of `1` and therefore the wallet creation time is always `1` and so the new logic doesn't actually skip any blocks. But if that's the case
...
💬 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-1563661471)
> To review this PR I wrote a simple functional test: https://github.com/pinheadmz/bitcoin/commit/1f0f945aca3a4f5118e49b2d31318ddd4b19d906 which surprised me by failing. I added logging and comments to the test to share it because I don't entirely understand what's happening but it looks to me like, the wallet generates a combo(...) descriptor with a timestamp of 1 and therefore the wallet creation time is always 1 and so the new logic doesn't actually skip any blocks. But if that's the case, do
...
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1563661471)
> To review this PR I wrote a simple functional test: https://github.com/pinheadmz/bitcoin/commit/1f0f945aca3a4f5118e49b2d31318ddd4b19d906 which surprised me by failing. I added logging and comments to the test to share it because I don't entirely understand what's happening but it looks to me like, the wallet generates a combo(...) descriptor with a timestamp of 1 and therefore the wallet creation time is always 1 and so the new logic doesn't actually skip any blocks. But if that's the case, do
...
💬 furszy 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-1563723413)
> I've fixed a bug which resulted in the conflicted tx not being marked as inactive if the conflicting tx was not in mapTxSpends.
Could you expand this?
Not sure if I follow the [latest push](https://github.com/bitcoin/bitcoin/compare/6f1d394896fcc7e2687b766418f9cd4e55b613b5..89df7987c2f1eea42454c2b0efc31a924fbfd3a8), where only the inequality was changed.
If `mapTxSpends.count(outpoint) == 1`, the outpoint was spent only once, so the wallet shouldn't contain any conflicting tx.
(https://github.com/bitcoin/bitcoin/pull/27145#issuecomment-1563723413)
> I've fixed a bug which resulted in the conflicted tx not being marked as inactive if the conflicting tx was not in mapTxSpends.
Could you expand this?
Not sure if I follow the [latest push](https://github.com/bitcoin/bitcoin/compare/6f1d394896fcc7e2687b766418f9cd4e55b613b5..89df7987c2f1eea42454c2b0efc31a924fbfd3a8), where only the inequality was changed.
If `mapTxSpends.count(outpoint) == 1`, the outpoint was spent only once, so the wallet shouldn't contain any conflicting tx.
💬 furszy commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1206163032)
yeah, great if that is already covered.
(sorry for the delayed answer)
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1206163032)
yeah, great if that is already covered.
(sorry for the delayed answer)