📝 glozow opened a pull request: "doc: mention bip94 support"
(https://github.com/bitcoin/bitcoin/pull/30655)
Followup to #29775, noticed while looking at #30604 and #30647. See [release process](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-every-major-and-minor-release).
(https://github.com/bitcoin/bitcoin/pull/30655)
Followup to #29775, noticed while looking at #30604 and #30647. See [release process](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-every-major-and-minor-release).
💬 maflcko commented on pull request "doc: mention bip94 support":
(https://github.com/bitcoin/bitcoin/pull/30655#issuecomment-2289070915)
ACK 99eeb51bf65fa00ee863f32d70f478bb9db0e351
(https://github.com/bitcoin/bitcoin/pull/30655#issuecomment-2289070915)
ACK 99eeb51bf65fa00ee863f32d70f478bb9db0e351
💬 paplorinc commented on pull request "coins: Add move operations to Coin and CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2289075127)
> configure clang-tidy to produce this warning in our own CI instead of Sonar
The ["coverage report"]( https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2284919165) already has the Sonarcloud section - I have opened this issue because of the #28280 corecheck [CI Sonar warnings](https://corecheck.dev/bitcoin/bitcoin/pulls/28280). Or do you mean we should be able to reproduce this locally as well via `clang-tidy`?
> if I want a class that is guaranteed to have a move constructor, I
...
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2289075127)
> configure clang-tidy to produce this warning in our own CI instead of Sonar
The ["coverage report"]( https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2284919165) already has the Sonarcloud section - I have opened this issue because of the #28280 corecheck [CI Sonar warnings](https://corecheck.dev/bitcoin/bitcoin/pulls/28280). Or do you mean we should be able to reproduce this locally as well via `clang-tidy`?
> if I want a class that is guaranteed to have a move constructor, I
...
📝 paplorinc opened a pull request: "coins: Simplify moves to ternary in `coins.cpp`"
(https://github.com/bitcoin/bitcoin/pull/30656)
Split out of https://github.com/bitcoin/bitcoin/pull/30643
See related discussions:
* https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676800505
* https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676801434
* https://github.com/bitcoin/bitcoin/pull/17487#discussion_r402037193
And reproducer that demonstrates the behavior of all 3 cases:
* https://gist.github.com/paplorinc/66f13938d867d82893d7ab7a2ebc5717
(https://github.com/bitcoin/bitcoin/pull/30656)
Split out of https://github.com/bitcoin/bitcoin/pull/30643
See related discussions:
* https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676800505
* https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676801434
* https://github.com/bitcoin/bitcoin/pull/17487#discussion_r402037193
And reproducer that demonstrates the behavior of all 3 cases:
* https://gist.github.com/paplorinc/66f13938d867d82893d7ab7a2ebc5717
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1717123190)
Addressed in https://github.com/bitcoin/bitcoin/pull/30656
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1717123190)
Addressed in https://github.com/bitcoin/bitcoin/pull/30656
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1717123576)
Addressed in https://github.com/bitcoin/bitcoin/pull/30656
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1717123576)
Addressed in https://github.com/bitcoin/bitcoin/pull/30656
💬 maflcko commented on pull request "coins: Add move operations to Coin and CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2289087961)
> > configure clang-tidy to produce this warning in our own CI instead of Sonar
>
> The ["coverage report"](https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2284919165) already has the Sonarcloud section - I have opened this issue because of the #28280 corecheck [CI Sonar warnings](https://corecheck.dev/bitcoin/bitcoin/pulls/28280). Or do you mean we should be able to reproduce this locally as well via `clang-tidy`?
>
> > if I want a class that is guaranteed to have a move constr
...
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2289087961)
> > configure clang-tidy to produce this warning in our own CI instead of Sonar
>
> The ["coverage report"](https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2284919165) already has the Sonarcloud section - I have opened this issue because of the #28280 corecheck [CI Sonar warnings](https://corecheck.dev/bitcoin/bitcoin/pulls/28280). Or do you mean we should be able to reproduce this locally as well via `clang-tidy`?
>
> > if I want a class that is guaranteed to have a move constr
...
💬 paplorinc commented on pull request "coins: allow write to disk without cache drop":
(https://github.com/bitcoin/bitcoin/pull/17487#discussion_r1717126291)
FYI: I've revived the if-vs-ternary for ::move in https://github.com/bitcoin/bitcoin/pull/30656
(https://github.com/bitcoin/bitcoin/pull/17487#discussion_r1717126291)
FYI: I've revived the if-vs-ternary for ::move in https://github.com/bitcoin/bitcoin/pull/30656
💬 sipa commented on pull request "coins: Add move operations to Coin and CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2289092299)
@maflcko `etd:is_nothrow_move_constructible_v` will also return true if a nothrow copy constructor is available (because a copy constructor is eligible in any context where a move constructor is called), so that isn't enough to verify that an efficient version exists.
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2289092299)
@maflcko `etd:is_nothrow_move_constructible_v` will also return true if a nothrow copy constructor is available (because a copy constructor is eligible in any context where a move constructor is called), so that isn't enough to verify that an efficient version exists.
💬 maflcko commented on pull request "coins: Simplify std::move to ternary in `coins.cpp`":
(https://github.com/bitcoin/bitcoin/pull/30656#discussion_r1717133218)
Can you explain what problem switching an `if` to ternary is solving? Linking to 3 discussion threads and one external gist, which may be deleted at any time seems not helpful to reviewers.
(https://github.com/bitcoin/bitcoin/pull/30656#discussion_r1717133218)
Can you explain what problem switching an `if` to ternary is solving? Linking to 3 discussion threads and one external gist, which may be deleted at any time seems not helpful to reviewers.
💬 fanquake commented on pull request "test: Fix test log file name":
(https://github.com/bitcoin/bitcoin/pull/30654#issuecomment-2289104629)
> However, src/test/README.md still refer
> This PR restores the pre-PR19385 behaviour,
Not sure this PR should be called a "fix"? As it seems the fix would be to just update the docs to match the current (intended) behaviour.
This instead seems to be a reversion to prior behaviour to facilitate something CMake related. I'm curious why this has only become an issue now, given we've already got the tests in CMake? Or is this something that was missing and is being ported now?
(https://github.com/bitcoin/bitcoin/pull/30654#issuecomment-2289104629)
> However, src/test/README.md still refer
> This PR restores the pre-PR19385 behaviour,
Not sure this PR should be called a "fix"? As it seems the fix would be to just update the docs to match the current (intended) behaviour.
This instead seems to be a reversion to prior behaviour to facilitate something CMake related. I'm curious why this has only become an issue now, given we've already got the tests in CMake? Or is this something that was missing and is being ported now?
💬 maflcko commented on pull request "test: Fix test log file name":
(https://github.com/bitcoin/bitcoin/pull/30654#issuecomment-2289116723)
Not sure about changing the test-only behavior here. Seems fine to just adjust the readme in the cmake pull, as the line has to be touched anyway. No need to be exact in porting this edge case?
(https://github.com/bitcoin/bitcoin/pull/30654#issuecomment-2289116723)
Not sure about changing the test-only behavior here. Seems fine to just adjust the readme in the cmake pull, as the line has to be touched anyway. No need to be exact in porting this edge case?
📝 theStack opened a pull request: "test: add functional test for XORed block/undo files (`-blockxor` option)"
(https://github.com/bitcoin/bitcoin/pull/30657)
This PR adds a dedicated functional test for XORed block data/undo file support (bitcoind option `-blocksxor`, see PR #28052). In order to verify that the XOR pattern has been applied, the {blk,rev}*.dat files are rewritten un-XORed manually by the test while the node is shut down; the node is then started again with `-blocksxor=0`, and both the data and undo files are verified via the `verifychain` RPC (with checklevel=2). Note that starting bitcoind with `-blocksxor=0` fails if a xor key is pr
...
(https://github.com/bitcoin/bitcoin/pull/30657)
This PR adds a dedicated functional test for XORed block data/undo file support (bitcoind option `-blocksxor`, see PR #28052). In order to verify that the XOR pattern has been applied, the {blk,rev}*.dat files are rewritten un-XORed manually by the test while the node is shut down; the node is then started again with `-blocksxor=0`, and both the data and undo files are verified via the `verifychain` RPC (with checklevel=2). Note that starting bitcoind with `-blocksxor=0` fails if a xor key is pr
...
💬 paplorinc commented on pull request "coins: Simplify std::move to ternary in `coins.cpp`":
(https://github.com/bitcoin/bitcoin/pull/30656#discussion_r1717166903)
You're right, added the following to the description of the pr:
> To avoid repetition and make the diff trivial between the two branches (calling the copy vs move constructors), this expression was originally written as a ternary, which unfortunately introduced an additional copy operation (and was reverted to a verbose if statement with a comment).
This change attempts to restore the signal to noise ratio of such a simple expression while retaining its performance.
(https://github.com/bitcoin/bitcoin/pull/30656#discussion_r1717166903)
You're right, added the following to the description of the pr:
> To avoid repetition and make the diff trivial between the two branches (calling the copy vs move constructors), this expression was originally written as a ternary, which unfortunately introduced an additional copy operation (and was reverted to a verbose if statement with a comment).
This change attempts to restore the signal to noise ratio of such a simple expression while retaining its performance.
💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2289142629)
Ok, I think this is easiest to solve by defining two fuzz targets, one that searches for invalid inputs (fast), and one that searches for valid inputs (slow).
This makes it possible to find the bug from https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2285605841 "immediately".
I've based this on a previous commit, with the bug included, so that it is trivial to test:
```
FUZZ=utxo_snapshot_invalid UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_s
...
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2289142629)
Ok, I think this is easiest to solve by defining two fuzz targets, one that searches for invalid inputs (fast), and one that searches for valid inputs (slow).
This makes it possible to find the bug from https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2285605841 "immediately".
I've based this on a previous commit, with the bug included, so that it is trivial to test:
```
FUZZ=utxo_snapshot_invalid UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_s
...
💬 achow101 commented on pull request "wallet: fix blank legacy detection":
(https://github.com/bitcoin/bitcoin/pull/30621#issuecomment-2289151498)
> Saw some `may be used uninitialized` warnings when building, e.g.
What compiler? I don't see those errors.
(https://github.com/bitcoin/bitcoin/pull/30621#issuecomment-2289151498)
> Saw some `may be used uninitialized` warnings when building, e.g.
What compiler? I don't see those errors.
💬 achow101 commented on pull request "doc: Deduplicate list of possible chain strings in RPC help texts":
(https://github.com/bitcoin/bitcoin/pull/30648#issuecomment-2289155780)
ACK 9b297555207b4ea54bc0051f09c7084797aa9def
(https://github.com/bitcoin/bitcoin/pull/30648#issuecomment-2289155780)
ACK 9b297555207b4ea54bc0051f09c7084797aa9def
💬 achow101 commented on pull request "wallet: fix blank legacy detection":
(https://github.com/bitcoin/bitcoin/pull/30621#issuecomment-2289158872)
ACK 6ed424f2db609f9f39ec1d1da2077c7616f3a0c2
(https://github.com/bitcoin/bitcoin/pull/30621#issuecomment-2289158872)
ACK 6ed424f2db609f9f39ec1d1da2077c7616f3a0c2
💬 TheCharlatan commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#issuecomment-2289173555)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30657#issuecomment-2289173555)
Concept ACK
🚀 achow101 merged a pull request: "doc: Deduplicate list of possible chain strings in RPC help texts"
(https://github.com/bitcoin/bitcoin/pull/30648)
(https://github.com/bitcoin/bitcoin/pull/30648)