💬 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?
💬 laanwj commented on pull request "guix: remove `util-linux`":
(https://github.com/bitcoin/bitcoin/pull/31285#issuecomment-2474381820)
Concept ACK
Does this make a difference to the build env?
i wonder if util-linux is somehow coming in through the closure of one of the other packages, it seems such a base thing
(https://github.com/bitcoin/bitcoin/pull/31285#issuecomment-2474381820)
Concept ACK
Does this make a difference to the build env?
i wonder if util-linux is somehow coming in through the closure of one of the other packages, it seems such a base thing
💬 maflcko commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2474383795)
Checked that `RUN_UNIT_TESTS=true MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_win64.sh" ./ci/test_run_all.sh` still works.
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2474383795)
Checked that `RUN_UNIT_TESTS=true MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_win64.sh" ./ci/test_run_all.sh` still works.
💬 instagibbs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2474385001)
ok replicated on this PR, I'll track it down
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2474385001)
ok replicated on this PR, I'll track it down
💬 marcofleon commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2474387376)
Yeah not on master, just this PR.
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2474387376)
Yeah not on master, just this PR.
💬 marcofleon commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2474394030)
It seems to be commit 7f0d8633b174c8b96cc28ec05005b8e89ab4957d
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2474394030)
It seems to be commit 7f0d8633b174c8b96cc28ec05005b8e89ab4957d
💬 marcofleon commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1840960818)
It's this commit here that's causing the errors. Seems that this was actually needed.
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1840960818)
It's this commit here that's causing the errors. Seems that this was actually needed.
💬 fanquake commented on pull request "guix: remove `util-linux`":
(https://github.com/bitcoin/bitcoin/pull/31285#issuecomment-2474401790)
> Does this make a difference to the build env?
Yea, it's pruning out`util-linux`. See here for a diff of `/bin` of the profile of master vs this PR: https://gist.github.com/fanquake/2785035aeab228a1d6046dbbdd55f299, for `x86_64-linux-gnu`.
(https://github.com/bitcoin/bitcoin/pull/31285#issuecomment-2474401790)
> Does this make a difference to the build env?
Yea, it's pruning out`util-linux`. See here for a diff of `/bin` of the profile of master vs this PR: https://gist.github.com/fanquake/2785035aeab228a1d6046dbbdd55f299, for `x86_64-linux-gnu`.
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#issuecomment-2474424857)
Just rebased this on master -- @0xB10C thanks for the note on the tracepoint change. @instagibbs I think I could rewrite the `CheckEphemeralSpends()` code to take advantage of the changesets for package validation (since we will now be maintaining a map of package transactions that we can query), but maybe that can be done in a separate PR? Just left that code as-is for now.
(https://github.com/bitcoin/bitcoin/pull/31122#issuecomment-2474424857)
Just rebased this on master -- @0xB10C thanks for the note on the tracepoint change. @instagibbs I think I could rewrite the `CheckEphemeralSpends()` code to take advantage of the changesets for package validation (since we will now be maintaining a map of package transactions that we can query), but maybe that can be done in a separate PR? Just left that code as-is for now.
💬 laanwj commented on pull request "guix: remove `util-linux`":
(https://github.com/bitcoin/bitcoin/pull/31285#issuecomment-2474441538)
Nice!
Of those, `hexdump` was required with the autotools build system. Not sure about others.
(https://github.com/bitcoin/bitcoin/pull/31285#issuecomment-2474441538)
Nice!
Of those, `hexdump` was required with the autotools build system. Not sure about others.
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1841026397)
Sure, looks like tests can be rewritten for it not to be needed. Will address in the next push
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1841026397)
Sure, looks like tests can be rewritten for it not to be needed. Will address in the next push
💬 polespinasa commented on pull request "Wallet: "listreceivedby*" fix":
(https://github.com/bitcoin/bitcoin/pull/30972#issuecomment-2474535023)
Lgtm
Tested ack
I ran the unit test and functional test (without bdb) and all pass.
Also tested manually using regtest the steps in function ``test_listreceivedby`` and the behaviour is the expected.
(https://github.com/bitcoin/bitcoin/pull/30972#issuecomment-2474535023)
Lgtm
Tested ack
I ran the unit test and functional test (without bdb) and all pass.
Also tested manually using regtest the steps in function ``test_listreceivedby`` and the behaviour is the expected.
👍 instagibbs approved a pull request: "cluster mempool: Implement changeset interface for mempool"
(https://github.com/bitcoin/bitcoin/pull/31122#pullrequestreview-2434231838)
ACK 5736d1ddacc4019101e7a5170dd25efbc63b622a
Picks up TRACEPOINT and ephemeral dust stuff.
via `git range-diff master 0ca1e05d8bcbc42892156c15f128a5f829e9e48e 5736d1ddacc4019101e7a5170dd25efbc63b622a`
(https://github.com/bitcoin/bitcoin/pull/31122#pullrequestreview-2434231838)
ACK 5736d1ddacc4019101e7a5170dd25efbc63b622a
Picks up TRACEPOINT and ephemeral dust stuff.
via `git range-diff master 0ca1e05d8bcbc42892156c15f128a5f829e9e48e 5736d1ddacc4019101e7a5170dd25efbc63b622a`
💬 instagibbs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1841060999)
specifically needs the `SyncWithValidationInterfaceQueue`, not the unused part.
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1841060999)
specifically needs the `SyncWithValidationInterfaceQueue`, not the unused part.