💬 maflcko commented on pull request "guix: accomodate migration to codeberg":
(https://github.com/bitcoin/bitcoin/pull/32439#discussion_r2108829918)
```suggestion
- A commit to skip this test is included since Guix 1.4.0:
```
(https://github.com/bitcoin/bitcoin/pull/32439#discussion_r2108829918)
```suggestion
- A commit to skip this test is included since Guix 1.4.0:
```
💬 fanquake commented on pull request "guix: accomodate migration to codeberg":
(https://github.com/bitcoin/bitcoin/pull/32439#discussion_r2108834816)
Thanks, added
(https://github.com/bitcoin/bitcoin/pull/32439#discussion_r2108834816)
Thanks, added
💬 maflcko commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2912031083)
> > I wonder if -reindex could be taught to do this instead.
>
> @maflcko That would be convenient, but a reindex is a much longer task than this tool. So I don't think "instead", but rather "also".
Maybe just from a code-review/maintenance perspective this is preferable, given that the feature implemented here is quite tricky to get right for all edge cases by (re-)implementing the C++ logic.
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2912031083)
> > I wonder if -reindex could be taught to do this instead.
>
> @maflcko That would be convenient, but a reindex is a much longer task than this tool. So I don't think "instead", but rather "also".
Maybe just from a code-review/maintenance perspective this is preferable, given that the feature implemented here is quite tricky to get right for all edge cases by (re-)implementing the C++ logic.
💬 maflcko commented on pull request "guix: accomodate migration to codeberg":
(https://github.com/bitcoin/bitcoin/pull/32439#issuecomment-2912033007)
lgtm ACK c8d9baae942c94d64ce47ae8f67d3710e6a296bd
(https://github.com/bitcoin/bitcoin/pull/32439#issuecomment-2912033007)
lgtm ACK c8d9baae942c94d64ce47ae8f67d3710e6a296bd
👍 TheCharlatan approved a pull request: "test: Add missing ipc subtree to lint"
(https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2870565681)
ACK fa2bf6bdb7e5f8940fe4bce1aaab29bf2e063b49
(https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2870565681)
ACK fa2bf6bdb7e5f8940fe4bce1aaab29bf2e063b49
👍 TheCharlatan approved a pull request: "test: fix pushdata scripts"
(https://github.com/bitcoin/bitcoin/pull/32270#pullrequestreview-2870576815)
Re-ACK f66b14d2ecb0db991d255be49d6ff6ec361569b5
(https://github.com/bitcoin/bitcoin/pull/32270#pullrequestreview-2870576815)
Re-ACK f66b14d2ecb0db991d255be49d6ff6ec361569b5
💬 hebasto commented on pull request "guix: accomodate migration to codeberg":
(https://github.com/bitcoin/bitcoin/pull/32439#discussion_r2108867196)
While this is correct, the mailing list [refers](https://lists.gnu.org/archive/html/info-guix/2025-05/msg00002.html) to `https://git.guix.gnu.org/guix.git` as the canonical URL. Should we use that instead?
(https://github.com/bitcoin/bitcoin/pull/32439#discussion_r2108867196)
While this is correct, the mailing list [refers](https://lists.gnu.org/archive/html/info-guix/2025-05/msg00002.html) to `https://git.guix.gnu.org/guix.git` as the canonical URL. Should we use that instead?
👍 TheCharlatan approved a pull request: "test: fix and augment block tests of invalid_txs"
(https://github.com/bitcoin/bitcoin/pull/32591#pullrequestreview-2870607751)
ACK 8fcd6845052354fad80ae7e5feda3f6a2e441e12
(https://github.com/bitcoin/bitcoin/pull/32591#pullrequestreview-2870607751)
ACK 8fcd6845052354fad80ae7e5feda3f6a2e441e12
🚀 fanquake merged a pull request: "test: fix pushdata scripts"
(https://github.com/bitcoin/bitcoin/pull/32270)
(https://github.com/bitcoin/bitcoin/pull/32270)
💬 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`.
(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.
(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?
(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
...
(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.
(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)
(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
...
(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
...
(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?
(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)`
(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
...
(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
...