💬 fanquake commented on pull request "guix: accomodate migration to codeberg":
(https://github.com/bitcoin/bitcoin/pull/32439#discussion_r2108890579)
Given that's just redirecting to `https://codeberg.org/guix/guix.git`, and the frequent issues/downtime we've seen lately, I'd rather not add a hop via `savannah.gnu.org`.

👍 hebasto approved a pull request: "guix: accomodate migration to codeberg"
(https://github.com/bitcoin/bitcoin/pull/32439#pullrequestreview-2870641995)
ACK c8d9baae942c94d64ce47ae8f67d3710e6a296bd.

My Guix build is coming.

💬 fanquake commented on issue "windows: depends config fails":
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2912140913)
@John-zhan does that work for you?

📝 brunoerg opened a pull request: "fuzz: wallet: add target for `MigrateToDescriptor`"
(https://github.com/bitcoin/bitcoin/pull/32624)
This PR adds fuzz coverage for the scriptpubkeyman migration (`MigrateToDescriptor`). Note that it's a test for the migration of the scriptpubkey manager, not for the whole migration process as tried in #29694, because:
1) The wallet migration deals with DBs which is expensive for fuzzing (was getting around 3 exec/s);
2) Mocking would require lots of refactors.

This target loads keys, HDChain (even inactive ones), watch only and might add tons of different scripts, then calls `MigrateToDes
...

💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2912181133)
Closing in favor of #32624.

✅ brunoerg closed a pull request: "fuzz: wallet: add target for spkm migration"
(https://github.com/bitcoin/bitcoin/pull/29694)

🤔 l0rinc requested changes to a pull request: "headerssync: Keep tests ahead of increasing params"
(https://github.com/bitcoin/bitcoin/pull/32579#pullrequestreview-2857912770)
Concept ACK – this PR replaces the imminently outdated hard-coded test target of 15'000 headers with a value derived from the live `REDOWNLOAD_BUFFER_SIZE` constant.
Moving both header-sync tuning constants into a shared header, and making the unit test reference them, keeps the test exercising the 'buffer-full' path as the parameters grow each release.

Recommendations:
* split the series into more commits, separating low-risk refactors from ones that change behaviour: strict move-only; ref
...

💬 l0rinc commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2108774510)
34e1513efeb193a46d37f184b9ab41bcda974afa:
I have inlined the methods and verified that the remaining changes are only comments, reused-and-cleared pointers and vectors 👍

<details>
<summary>A few more refactoring nits that could be considered to make this test even more readable and consistent</summary>

```diff
diff --git a/src/test/headers_sync_chainwork_tests.cpp b/src/test/headers_sync_chainwork_tests.cpp
index 054853bbbf..89bb6682d1 100644
--- a/src/test/headers_sync_chainwork_te
...

💬 l0rinc commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2108817538)
This seems to be the only place where `request_more` isn't validated.

nit: what's the meaning of "foiled" here?

💬 l0rinc commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2100413257)
I know this is just a move, but instead of just bare magic number + magic comment (which also has to be [manually updated every time](https://github.com/bitcoin/bitcoin/commit/11a2d3a63e90cdc1920ede3c67d52a9c72860e6b)), could we have the something like : `Generated by headerssync-params.py (on 2025-03-04)`

💬 l0rinc commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2108808636)
we're reusing the previous `result` (which was brace initialized), what if we used braces to delimit the two independent code parts (+ a few other nits you might want to consider):
```suggestion
// Verify that just trying to process the second chain would not succeed (too little work).
HeadersSyncState hss{0, Params().GetConsensus(), chain_start, CHAIN_WORK};
BOOST_REQUIRE_EQUAL(hss.GetState(), HeadersSyncState::State::PRESYNC);
// Pretend just the first message is "full", so we don't abort
...

💬 l0rinc commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2108831717)
`size_t` might me more fitting here since `GenerateHeaders` accepts that instead of an `int`:
```suggestion
constexpr size_t TARGET_BLOCKS{15'000};
```

💬 l0rinc commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2108830588)
should we make any of these `explicit`?

💬 l0rinc commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2108846074)
👍 for comparing size instead of empty - the failure message would likely be more meaningful

💬 l0rinc commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2108851720)
I think we could do these in the previous refactor commit instead - to separate low risk changes from ones where we have to may more attention, while keeping the first one about move-only changes + minor ones. Or do the first commit as strictly move-only and add a second one with these tiny refactors (I'd like the latter more).

💬 l0rinc commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2108844548)
Could we use `const std::span<const CBlockHeader>` in these helpers as well? And maybe split these long lines.

👍 l0rinc approved a pull request: "test: Add missing ipc subtree to lint"
(https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2870746798)
Concept ACK, left a consistency nit only

💬 l0rinc commented on pull request "test: Add missing ipc subtree to lint":
(https://github.com/bitcoin/bitcoin/pull/32623#discussion_r2108973482)
visiting the links will automatically remove the `.git` suffixes (which was already missing from `libmultiprocess`):
```suggestion
* for `src/crc32c`: https://github.com/bitcoin-core/crc32c-subtree (branch `bitcoin-fork`)
* for `src/crypto/ctaes`: https://github.com/bitcoin-core/ctaes (branch `master`)
* for `src/ipc/libmultiprocess`: https://github.com/bitcoin-core/libmultiprocess (branch `master`)
* for `src/leveldb`: https://github.com/bitcoin-core/leveldb-subtree (branch `bitcoin-fork`)
...

💬 maflcko commented on issue "windows: depends config fails":
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2912220775)
An alternative would be to just not use cmake 4.x and stick with cmake 3.x for now?

🤔 TheCharlatan reviewed a pull request: "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage"
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2870684474)
Concept ACK