👍 pablomartin4btc approved a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2433754801)
re-ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048
Since my last review: removal of `g_rng_temp_path`, renaming and moving definition of `SetupCommonTestArgs` to `src/test/util/setup_common.h` and using the timestamp for the test dir path instead of a random id.
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2433754801)
re-ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048
Since my last review: removal of `g_rng_temp_path`, renaming and moving definition of `SetupCommonTestArgs` to `src/test/util/setup_common.h` and using the timestamp for the test dir path instead of a random id.
💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1840743103)
The `'` are used to denote the begin and the end of the string, which would otherwise not be possible, because trailing spaces can normally not be seen when printing. They are not needed in this test and they are a leftover when I tested this against tinyformat at runtime.
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1840743103)
The `'` are used to denote the begin and the end of the string, which would otherwise not be possible, because trailing spaces can normally not be seen when printing. They are not needed in this test and they are a leftover when I tested this against tinyformat at runtime.
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1840747719)
pushed a lightly modified version that also reports the child txid on failure, rather than guessing it's the ultimate tx in the package. thanks! https://github.com/bitcoin/bitcoin/pull/31279
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1840747719)
pushed a lightly modified version that also reports the child txid on failure, rather than guessing it's the ultimate tx in the package. thanks! https://github.com/bitcoin/bitcoin/pull/31279
👋 instagibbs's pull request is ready for review: "policy: ephemeral dust followups"
(https://github.com/bitcoin/bitcoin/pull/31279)
(https://github.com/bitcoin/bitcoin/pull/31279)
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1840754607)
> 11:24 <@dergoegge> I've suggested `LIMITED_WHILE(provider.remaining_bytes() > 0, iter_limit)` in the past as well
11:24 <@dergoegge> I think the goal is to let the fuzzer "choose" the number of iterations and both the above and consumebool achieve that
11:24 <@dergoegge> Iirc there were concerns around using remaining_bytes as really large inputs might cause timeouts
11:24 <@dergoegge> which is something we've seen with consumebool as well if the iteration limit is too high
I'm going to
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1840754607)
> 11:24 <@dergoegge> I've suggested `LIMITED_WHILE(provider.remaining_bytes() > 0, iter_limit)` in the past as well
11:24 <@dergoegge> I think the goal is to let the fuzzer "choose" the number of iterations and both the above and consumebool achieve that
11:24 <@dergoegge> Iirc there were concerns around using remaining_bytes as really large inputs might cause timeouts
11:24 <@dergoegge> which is something we've seen with consumebool as well if the iteration limit is too high
I'm going to
...
💬 instagibbs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2474116685)
ready for review, I think
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2474116685)
ready for review, I think
💬 achow101 commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840779076)
It returned RPC originally, and I don't think that adding a rename to the diff would be helpful.
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840779076)
It returned RPC originally, and I don't think that adding a rename to the diff would be helpful.
💬 achow101 commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840781994)
No, the master node migrates a copy of the wallet. The old node will have the original copy which remains unchanged regardless.
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840781994)
No, the master node migrates a copy of the wallet. The old node will have the original copy which remains unchanged regardless.
📝 maflcko opened a pull request: "ci: Skip broken Wine64 tests by default"
(https://github.com/bitcoin/bitcoin/pull/31284)
I don't think the unit tests run in Wine after the Windows cross-compilation have ever shown a true positive since the MSVC task was added. However, they are a source of frequent false-positives.
Thus, disable them by default for now. Anyone can still enable them by setting `RUN_UNIT_TESTS=true`.
Fixes #31071
A follow-up could run them on real Windows, but this comes with its own downsides, see https://github.com/bitcoin/bitcoin/pull/31176.
Conceptually there are many other nightly t
...
(https://github.com/bitcoin/bitcoin/pull/31284)
I don't think the unit tests run in Wine after the Windows cross-compilation have ever shown a true positive since the MSVC task was added. However, they are a source of frequent false-positives.
Thus, disable them by default for now. Anyone can still enable them by setting `RUN_UNIT_TESTS=true`.
Fixes #31071
A follow-up could run them on real Windows, but this comes with its own downsides, see https://github.com/bitcoin/bitcoin/pull/31176.
Conceptually there are many other nightly t
...
💬 achow101 commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840786362)
`notloaded` is specifically tested because legacy wallets won't be able to be loaded.
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840786362)
`notloaded` is specifically tested because legacy wallets won't be able to be loaded.
💬 achow101 commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840787871)
Yes, no test relies on the time.
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840787871)
Yes, no test relies on the time.
💬 achow101 commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840789029)
Will do if I need to retouch.
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840789029)
Will do if I need to retouch.
💬 achow101 commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840789191)
Will do if I need to retouch.
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840789191)
Will do if I need to retouch.
💬 achow101 commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840789364)
Will do if I need to retouch.
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840789364)
Will do if I need to retouch.
💬 sdaftuar commented on pull request "validation: Remove RECENT_CONSENSUS_CHANGE validation result":
(https://github.com/bitcoin/bitcoin/pull/31269#issuecomment-2474285264)
As far as I can tell, I was also in agreement with not needing this `RECENT_CONSENSUS_CHANGE` enum in our code until it was time to use it! @TheBlueMatt was the one who thought we needed it, and I came around to thinking that there was a potential use case for it in the event of another soft fork.
But I'm fine with dropping it; if someone decides to rework our net_processing code to take advantage of subtler differences in validation failures, we can reintroduce it at that point.
(https://github.com/bitcoin/bitcoin/pull/31269#issuecomment-2474285264)
As far as I can tell, I was also in agreement with not needing this `RECENT_CONSENSUS_CHANGE` enum in our code until it was time to use it! @TheBlueMatt was the one who thought we needed it, and I came around to thinking that there was a potential use case for it in the event of another soft fork.
But I'm fine with dropping it; if someone decides to rework our net_processing code to take advantage of subtler differences in validation failures, we can reintroduce it at that point.
🤔 maflcko reviewed a pull request: "doc: Fix gen-manpages to check build options"
(https://github.com/bitcoin/bitcoin/pull/29457#pullrequestreview-2433974717)
This just fails on the release binaries. Please make sure to test the code yourself before asking for review. If you run into issues, you can ask questions.
```
BUILDDIR=bld ./contrib/devtools/gen-manpages.py
Aborting generating manpages...
Error: 'HAVE_SYSTEM' (System component) support is not enabled.
Please enable it and try again.
```
You'll probably have to use `bld/src/bitcoin-build-config.h` instead of the test ini. It seems odd anyway to tie the test config to this.
(https://github.com/bitcoin/bitcoin/pull/29457#pullrequestreview-2433974717)
This just fails on the release binaries. Please make sure to test the code yourself before asking for review. If you run into issues, you can ask questions.
```
BUILDDIR=bld ./contrib/devtools/gen-manpages.py
Aborting generating manpages...
Error: 'HAVE_SYSTEM' (System component) support is not enabled.
Please enable it and try again.
```
You'll probably have to use `bld/src/bitcoin-build-config.h` instead of the test ini. It seems odd anyway to tie the test config to this.
💬 maflcko commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1840887318)
You can't just change the relative path to include an unrelated prefix. This breaks when using `BUILDDIR`. Please see the conflicting pull request on how to correctly do it.
(https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1840887318)
You can't just change the relative path to include an unrelated prefix. This breaks when using `BUILDDIR`. Please see the conflicting pull request on how to correctly do it.
📝 fanquake opened a pull request: "guix: remove `util-linux`"
(https://github.com/bitcoin/bitcoin/pull/31285)
It's not immediately clear which of the utils was the last used (maybe getopt, will check), and when it was last used (maybe post CMake), but this package is no-longer needed to complete a Guix build (or needed in the CI). `util-linux` has been in the manifest since Guix was first introduced (3e80ec3ea9691c7c89173de922a113e643fe976b).
Guix build:
```bash
b681575760a0d19445a17d0d54f50a65d705923caee3a7cd7502e6611950dc11 guix-build-4d668549825c/output/aarch64-linux-gnu/SHA256SUMS.part
7af142
...
(https://github.com/bitcoin/bitcoin/pull/31285)
It's not immediately clear which of the utils was the last used (maybe getopt, will check), and when it was last used (maybe post CMake), but this package is no-longer needed to complete a Guix build (or needed in the CI). `util-linux` has been in the manifest since Guix was first introduced (3e80ec3ea9691c7c89173de922a113e643fe976b).
Guix build:
```bash
b681575760a0d19445a17d0d54f50a65d705923caee3a7cd7502e6611950dc11 guix-build-4d668549825c/output/aarch64-linux-gnu/SHA256SUMS.part
7af142
...
💬 marcofleon commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2474371365)
From various fuzz runs:
<details>
<summary>stack trace 1</summary>
```
UndefinedBehaviorSanitizer:DEADLYSIGNAL
==1654160==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000010 (pc 0x7f4ed8ac0563 bp 0x7ffd7bdeff90 sp 0x7ffd7bdefba8 T1654160)
==1654160==The signal is caused by a READ memory access.
==1654160==Hint: address points to the zero page.
#0 0x7f4ed8ac0563 in std::_Rb_tree_rebalance_for_erase(std::_Rb_tree_node_base*, std::_Rb_tree_node_base&) (/lib/x86
...
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2474371365)
From various fuzz runs:
<details>
<summary>stack trace 1</summary>
```
UndefinedBehaviorSanitizer:DEADLYSIGNAL
==1654160==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000010 (pc 0x7f4ed8ac0563 bp 0x7ffd7bdeff90 sp 0x7ffd7bdefba8 T1654160)
==1654160==The signal is caused by a READ memory access.
==1654160==Hint: address points to the zero page.
#0 0x7f4ed8ac0563 in std::_Rb_tree_rebalance_for_erase(std::_Rb_tree_node_base*, std::_Rb_tree_node_base&) (/lib/x86
...
💬 instagibbs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2474376868)
@marcofleon to be clear this is happening on master or follow-up?
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2474376868)
@marcofleon to be clear this is happening on master or follow-up?