💬 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,
...

💬 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
...

💬 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.

💬 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.

💬 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.

💬 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`
...

💬 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.

👍 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
...

💬 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.

🤔 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

💬 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?

💬 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?

💬 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?

💬 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

💬 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.

💬 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.

💬 achow101 commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3353204914)
ACK fc861332b351c9390400054ff74193ce26eb0713

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392383472)
I don't think it is necessary to check this, we can assume that libsecp is working correctly.

🤔 mzumsande reviewed a pull request: "p2p: Mitigate GETADDR fingerprinting by setting address timestamps to a fixed value"
(https://github.com/bitcoin/bitcoin/pull/33498#pullrequestreview-3286046607)
Concept ACK

The suggested solution reduces the precision of timestamps for the receiving party, but makes fingerprinting harder - seems like an acceptable trade-off to me.

If there is an active node behind the address it won't be `Terrible` for another ~20 days - until that happens, we could update it with a more accurate timestamp when we connect to it or receive the addr via gossip relay.

💬 mzumsande commented on pull request "p2p: Mitigate GETADDR fingerprinting by setting address timestamps to a fixed value":
(https://github.com/bitcoin/bitcoin/pull/33498#discussion_r2392350010)
Since this is calculated on a level of minutes, it looks like it would give a range 10.5±2.5 days, not 10±2 days.