💬 ismaelsadeeq commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3352926498)
Approach ACK
I ran this branch for a while on mainnet and have not noticed any issue, importing of mempool saved before cluster succeeded in multiple runs.
> Would it be a good idea to split this PR into a main PR with Cluster Implementation + Cleanups + Docs and Tests, then a followups PR with Optimizations and more cleanups? Goal is to get everything in v31, but this might help with reducing the size of the main PR and make it easier to punt cleanup-related comments to later.
I've sp
...
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3352926498)
Approach ACK
I ran this branch for a while on mainnet and have not noticed any issue, importing of mempool saved before cluster succeeded in multiple runs.
> Would it be a good idea to split this PR into a main PR with Cluster Implementation + Cleanups + Docs and Tests, then a followups PR with Optimizations and more cleanups? Goal is to get everything in v31, but this might help with reducing the size of the main PR and make it easier to punt cleanup-related comments to later.
I've sp
...
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392172151)
The `-disablewallet` error was caused by `self.uses_wallet = True`, not MiniWallet. The test framework starts nodes with `-disablewallet` when `uses_wallet = True` in no-wallet CI environments. MiniWallet doesn't need wallet RPC (it uses `sendrawtransaction`), so changing to `uses_wallet = None` allows the test to run in no-wallet environments while still using MiniWallet for transaction creation.
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392172151)
The `-disablewallet` error was caused by `self.uses_wallet = True`, not MiniWallet. The test framework starts nodes with `-disablewallet` when `uses_wallet = True` in no-wallet CI environments. MiniWallet doesn't need wallet RPC (it uses `sendrawtransaction`), so changing to `uses_wallet = None` allows the test to run in no-wallet environments while still using MiniWallet for transaction creation.
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392173987)
Recent changes uses a different approach, so there is no need for this anymore
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392173987)
Recent changes uses a different approach, so there is no need for this anymore
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3352950893)
> > they weren't legacy wallet specific but rather configuration problems.
>
> You'll have to adjust the pull description. Also, CI fails.
>
> To ensure this is tested once, you can add `--gen-test-data` to one CI task config via TEST_RUNNER_EXTRA.
Updated the PR title and description
The recent simplification changes make adding a CI task config unnecessary. The test now always generates data with MiniWallet by default, so the data generation path is automatically tested in all CI e
...
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3352950893)
> > they weren't legacy wallet specific but rather configuration problems.
>
> You'll have to adjust the pull description. Also, CI fails.
>
> To ensure this is tested once, you can add `--gen-test-data` to one CI task config via TEST_RUNNER_EXTRA.
Updated the PR title and description
The recent simplification changes make adding a CI task config unnecessary. The test now always generates data with MiniWallet by default, so the data generation path is automatically tested in all CI e
...
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3352960402)
> I think this test could be simplified to always generate data with the MiniWallet and test with generated data, instead of dumping it to a file and loading it later. I don't see the advantage of "generate" and "load" paths, and it's creating unnecessary complexity.
> I think this test could be simplified to always generate data with the MiniWallet and test with generated data, instead of dumping it to a file and loading it later. I don't see the advantage of "generate" and "load" paths,
...
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3352960402)
> I think this test could be simplified to always generate data with the MiniWallet and test with generated data, instead of dumping it to a file and loading it later. I don't see the advantage of "generate" and "load" paths, and it's creating unnecessary complexity.
> I think this test could be simplified to always generate data with the MiniWallet and test with generated data, instead of dumping it to a file and loading it later. I don't see the advantage of "generate" and "load" paths,
...
💬 m3dwards commented on pull request "ci: fix buildx gha cache authentication on forks":
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3352975826)
ACK bc706955d740f8a59bec78e44d33e80d1cca373b
> Fork run ongoing at: https://github.com/willcl-ark/bitcoin/actions/runs/18126864863
As it wasn't master, I've also pushed to my master to also check writing to the cache: https://github.com/m3dwards/bitcoin/actions/runs/18136688725/job/51617223612
And I can see all is well:
```shell
#12 exporting to GitHub Actions Cache
#12 preparing build cache for export
#12 writing layer sha256:a671ea418149154b4e0f1d5670be077da1a48f411402479d8fc137
...
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3352975826)
ACK bc706955d740f8a59bec78e44d33e80d1cca373b
> Fork run ongoing at: https://github.com/willcl-ark/bitcoin/actions/runs/18126864863
As it wasn't master, I've also pushed to my master to also check writing to the cache: https://github.com/m3dwards/bitcoin/actions/runs/18136688725/job/51617223612
And I can see all is well:
```shell
#12 exporting to GitHub Actions Cache
#12 preparing build cache for export
#12 writing layer sha256:a671ea418149154b4e0f1d5670be077da1a48f411402479d8fc137
...
💬 jonatack commented on pull request "p2p: Use network-dependent timers for inbound inv scheduling":
(https://github.com/bitcoin/bitcoin/pull/33464#issuecomment-3352985083)
Concept ACK. Good to see proposals like this and https://github.com/bitcoin/bitcoin/pull/33498 to address fingerprinting.
(https://github.com/bitcoin/bitcoin/pull/33464#issuecomment-3352985083)
Concept ACK. Good to see proposals like this and https://github.com/bitcoin/bitcoin/pull/33498 to address fingerprinting.
💬 jonatack commented on pull request "p2p: Mitigate GETADDR fingerprinting by setting address timestamps to a fixed value":
(https://github.com/bitcoin/bitcoin/pull/33498#issuecomment-3352992312)
Concept ACK on working on this. Good to see proposals like this and #33464.
(https://github.com/bitcoin/bitcoin/pull/33498#issuecomment-3352992312)
Concept ACK on working on this. Good to see proposals like this and #33464.
💬 ajtowns commented on issue "[29.x] guix build failure on ppc with xcb":
(https://github.com/bitcoin/bitcoin/issues/33488#issuecomment-3353006087)
No? I just untarred the Xcode SDK in a clean environment, set HOSTS and ran guix-build. The depends/sources files all seem to match the hashes from depends/packages as far as I can see for what that's worth.
(https://github.com/bitcoin/bitcoin/issues/33488#issuecomment-3353006087)
No? I just untarred the Xcode SDK in a clean environment, set HOSTS and ran guix-build. The depends/sources files all seem to match the hashes from depends/packages as far as I can see for what that's worth.
💬 maflcko commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3353039818)
> **Changes:**
>
> * Changed `self.uses_wallet = True` to `self.uses_wallet = None` to allow no-wallet environments
>
> * Added `self.skip_if_no_wallet()` when generating test data to ensure wallet is available when needed
>
> * Added `--gen-test-data` to TSan CI via `TEST_RUNNER_EXTRA` to ensure data generation is tested
>
>
> **How it works:**
>
> * In no-wallet environments: Test loads pre-generated data (no wallet required)
>
> * With `--gen-test-data`
...
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3353039818)
> **Changes:**
>
> * Changed `self.uses_wallet = True` to `self.uses_wallet = None` to allow no-wallet environments
>
> * Added `self.skip_if_no_wallet()` when generating test data to ensure wallet is available when needed
>
> * Added `--gen-test-data` to TSan CI via `TEST_RUNNER_EXTRA` to ensure data generation is tested
>
>
> **How it works:**
>
> * In no-wallet environments: Test loads pre-generated data (no wallet required)
>
> * With `--gen-test-data`
...
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3353093426)
> Generally, instead of just repeating the changes made, it could make sense to motivate them and explain why each change makes sense and is correct.
Updated PR description to explain motivation rather than just listing changes.
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3353093426)
> Generally, instead of just repeating the changes made, it could make sense to motivate them and explain why each change makes sense and is correct.
Updated PR description to explain motivation rather than just listing changes.
👍 stickies-v approved a pull request: "[28.x] backports + 28.3rc1"
(https://github.com/bitcoin/bitcoin/pull/33476#pullrequestreview-3285604677)
ACK dcac36271f9c6d47a863ab638a91382be6f7a50f , although I'm a bit confused about one change (see comment) that doesn't seem like a backport (but not necessarily bad by itself).
I verified all backports are clean, except for a bunch of test-only merge conflicts in 33106:
- cf875f15596052d42abccecb81a96d8e343fd2cd backported from 72dc18467dbfc16cdbda2dd109b087243b397799: addressed merge conflict from c16ae717689295338025dde74c0a7a51965c383f and re-added `-datacarriersize` arg because of 9f36
...
(https://github.com/bitcoin/bitcoin/pull/33476#pullrequestreview-3285604677)
ACK dcac36271f9c6d47a863ab638a91382be6f7a50f , although I'm a bit confused about one change (see comment) that doesn't seem like a backport (but not necessarily bad by itself).
I verified all backports are clean, except for a bunch of test-only merge conflicts in 33106:
- cf875f15596052d42abccecb81a96d8e343fd2cd backported from 72dc18467dbfc16cdbda2dd109b087243b397799: addressed merge conflict from c16ae717689295338025dde74c0a7a51965c383f and re-added `-datacarriersize` arg because of 9f36
...
💬 stickies-v commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2392038207)
The commit from which this is backported just added an extra zero:
https://github.com/bitcoin/bitcoin/commit/6da5de58cabc4133c379baa50845e30e5bc6b3e4#diff-63dc84ee23b871d1f4931c4f261b3c6b815bd8d5a65098a61bce8ea9b6b81965
What's the reason for changing that to dividing by 2 here? It also doesn't seem necessary for the test to complete.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2392038207)
The commit from which this is backported just added an extra zero:
https://github.com/bitcoin/bitcoin/commit/6da5de58cabc4133c379baa50845e30e5bc6b3e4#diff-63dc84ee23b871d1f4931c4f261b3c6b815bd8d5a65098a61bce8ea9b6b81965
What's the reason for changing that to dividing by 2 here? It also doesn't seem necessary for the test to complete.
🤔 maflcko reviewed a pull request: "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py"
(https://github.com/bitcoin/bitcoin/pull/33184#pullrequestreview-3285972950)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
(https://github.com/bitcoin/bitcoin/pull/33184#pullrequestreview-3285972950)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
💬 maflcko commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392299387)
the file can be deleted now?
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392299387)
the file can be deleted now?
💬 maflcko commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392301282)
method can be deleted now?
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392301282)
method can be deleted now?
💬 maflcko commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392301711)
what is this for?
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392301711)
what is this for?
💬 maflcko commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392309486)
why make this uppercase? could just leave as-is to make the diff smaller
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392309486)
why make this uppercase? could just leave as-is to make the diff smaller
💬 glozow commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2392363907)
I think you are right that changing the `Decimal` would have worked - apologies for the confusing change in approach.
This test doesn't exist on master, so it isn't part of the backport. It's a conflict resolution - the test was adapted to make it pass on that commit. I think you're referring to a different line lower down (L315) where it's backported and adds a 0.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2392363907)
I think you are right that changing the `Decimal` would have worked - apologies for the confusing change in approach.
This test doesn't exist on master, so it isn't part of the backport. It's a conflict resolution - the test was adapted to make it pass on that commit. I think you're referring to a different line lower down (L315) where it's backported and adds a 0.
💬 glozow commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#issuecomment-3353195323)
> nit: https://github.com/bitcoin/bitcoin/commit/e3273e03b1aea0c3ee3aeca802c984ab007f91ed and https://github.com/bitcoin/bitcoin/commit/08eeb0d3423cdfb6110794dd2bfdd5571459f751 have two spaces after Rebased-From: instead of the usual one, I'm not sure if that'd break any tooling (it did for me, but was an easy fix)
Sorry about that! I hadn't really thought about the number of spaces, but will keep it in mind in the future.
(https://github.com/bitcoin/bitcoin/pull/33476#issuecomment-3353195323)
> nit: https://github.com/bitcoin/bitcoin/commit/e3273e03b1aea0c3ee3aeca802c984ab007f91ed and https://github.com/bitcoin/bitcoin/commit/08eeb0d3423cdfb6110794dd2bfdd5571459f751 have two spaces after Rebased-From: instead of the usual one, I'm not sure if that'd break any tooling (it did for me, but was an easy fix)
Sorry about that! I hadn't really thought about the number of spaces, but will keep it in mind in the future.