💬 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)
📝 nowyouseeithaha opened a pull request: "Patch-1"
(https://github.com/bitcoin/bitcoin/pull/27764)
(https://github.com/bitcoin/bitcoin/pull/27764)
✅ pinheadmz closed a pull request: "Patch-1"
(https://github.com/bitcoin/bitcoin/pull/27764)
(https://github.com/bitcoin/bitcoin/pull/27764)
📝 fanquake locked a pull request: "Patch-1"
(https://github.com/bitcoin/bitcoin/pull/27764)
(https://github.com/bitcoin/bitcoin/pull/27764)
💬 ajtowns commented on pull request "p2p: Log addresses of stalling peers":
(https://github.com/bitcoin/bitcoin/pull/27761#discussion_r1206228638)
Should be `Peer=%d%s` (the result of strprintf isn't a number) and `peeraddr=%s` (for better consistency with other `fLogIPs` logs)
(https://github.com/bitcoin/bitcoin/pull/27761#discussion_r1206228638)
Should be `Peer=%d%s` (the result of strprintf isn't a number) and `peeraddr=%s` (for better consistency with other `fLogIPs` logs)
💬 mzumsande commented on pull request "p2p: Log addresses of stalling peers":
(https://github.com/bitcoin/bitcoin/pull/27761#discussion_r1206242431)
fixed, thanks.
(https://github.com/bitcoin/bitcoin/pull/27761#discussion_r1206242431)
fixed, thanks.
💬 MarcoFalke commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1563846514)
> It also provides an easy way to see when calls to shutdown are made without returning to the calling function.
Can you explain this? Am I correct when guessing that any added EXIT* macro, and not the BUBBLE* macros are indicating a missing return?
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1563846514)
> It also provides an easy way to see when calls to shutdown are made without returning to the calling function.
Can you explain this? Am I correct when guessing that any added EXIT* macro, and not the BUBBLE* macros are indicating a missing return?
📝 MarcoFalke opened a pull request: "test: Throw error when -signetchallenge is non-hex"
(https://github.com/bitcoin/bitcoin/pull/27765)
Instead of silently parsing non-hex to an empty challenge, throw an error.
Also, add missing includes while touching the file.
(https://github.com/bitcoin/bitcoin/pull/27765)
Instead of silently parsing non-hex to an empty challenge, throw an error.
Also, add missing includes while touching the file.
💬 MarcoFalke commented on pull request "test: Move test_chain_listunspent wallet check from mempool_packages to wallet_basic":
(https://github.com/bitcoin/bitcoin/pull/27735#issuecomment-1563922719)
Thx, done
(https://github.com/bitcoin/bitcoin/pull/27735#issuecomment-1563922719)
Thx, done
💬 MarcoFalke commented on pull request "doc: Add doc/release-notes/release-notes-25.0.md":
(https://github.com/bitcoin/bitcoin/pull/27751#discussion_r1206361499)
```suggestion
- Transactions of non-witness size 65 bytes and above are now allowed by mempool
```
(https://github.com/bitcoin/bitcoin/pull/27751#discussion_r1206361499)
```suggestion
- Transactions of non-witness size 65 bytes and above are now allowed by mempool
```
💬 MarcoFalke commented on pull request "doc: Add doc/release-notes/release-notes-25.0.md":
(https://github.com/bitcoin/bitcoin/pull/27751#discussion_r1206363521)
why? There is no list of all pulls in the notes, so the number is without context?
(https://github.com/bitcoin/bitcoin/pull/27751#discussion_r1206363521)
why? There is no list of all pulls in the notes, so the number is without context?
💬 MarcoFalke commented on pull request "doc: Add doc/release-notes/release-notes-25.0.md":
(https://github.com/bitcoin/bitcoin/pull/27751#issuecomment-1563956991)
Fixed the broken link. Not sure about the other nits.
(https://github.com/bitcoin/bitcoin/pull/27751#issuecomment-1563956991)
Fixed the broken link. Not sure about the other nits.