👍 dergoegge approved a pull request: "consensus: Remove checkpoints (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2592286198)
ACK 301017c6217378ca868e17c7cb8ffe3447abb11d

Only a comment changed since my last review (https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2550177263).

💬 fanquake commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2633390450)
> If there are any easier steps to reproduce this maybe using docker,

Running `time env -i HOME="$HOME" PATH="$PATH" USER="$USER" bash -c 'FILE_ENV="./ci/test/00_setup_env_i686_multiprocess.sh" ./ci/test_run_all.sh'` with the branch from #29796.

> or just a CI run I can look at that would be helpful.

https://github.com/bitcoin/bitcoin/pull/29796/checks?check_run_id=36485685971

🚀 fanquake merged a pull request: "lint: Call more checks from test_runner"
(https://github.com/bitcoin/bitcoin/pull/31653)

👍 dergoegge approved a pull request: "multi-peer orphan resolution followups"
(https://github.com/bitcoin/bitcoin/pull/31666#pullrequestreview-2592314173)
Code review ACK 7426afbe62414fa575f91b4f8d3ea63bcc653e8b

💬 Sjors commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2633426678)
I'm unable to make a Guix build. I'm using https://github.com/Sjors/bitcoin/commits/2025/02/ipc-guix/ which enables multiprocess in the simplest possible way, without modifying the depends system, by setting `MULTIPROCESS=1` in the pre-download and depends make step.

It downloads and builds capnp as expected, but in the download phase it says:

```
Checksum missing or mismatched for native_libmultiprocess source. Forcing re-download.
```

Which seems strange.

And then during the (off
...

🚀 fanquake merged a pull request: "multi-peer orphan resolution followups"
(https://github.com/bitcoin/bitcoin/pull/31666)

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1940887132)
Policy is what we put in blocks by default (`DEFAULT_BLOCK_MAX_WEIGHT`). If the user wants to override the default using `-blockmaxweight`, they should only be limited by consensus (`MAX_BLOCK_WEIGHT`).

> but the generated block would still be checked against policy, meaning their higher value would have no effect.

That would be a bug.

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2633492410)
Here's a patch that limits the use of `DEFAULT_BLOCK_MAX_WEIGHT` to defaults:

```diff
diff --git a/src/init.cpp b/src/init.cpp
index 41db01b129..f9eb466408 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -19,6 +19,7 @@
#include <common/args.h>
#include <common/system.h>
#include <consensus/amount.h>
+#include <consensus/consensus.h>
#include <deploymentstatus.h>
#include <hash.h>
#include <httprpc.h>
@@ -1018,15 +1019,15 @@ bool AppInitParameterInteraction(const ArgsManager
...

💬 dergoegge commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2633512621)
Concept ACK

I would suggest that you [squash](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits) your commits (as you are fixing up the first commit in the second one).

Just as a note, there's also `test/functional/feature_taproot.py` which produces [taproot unit tests](https://raw.githubusercontent.com/bitcoin-core/qa-assets/refs/heads/main/unit_test_data/script_assets_test.json). I'm not sure if those were meant to be extended in the case of Tapscript updat
...

👍 rkrux approved a pull request: "wallet: Automatically repair corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124#pullrequestreview-2592149096)
tACK e92dc1a3d36be02a8b16eed26159a43cfbe3dbcc

Thanks for this PR, I enjoyed reviewing it. I ran the tests and debugged the compatibility test.

Can / Will this also be back-ported to the affected version or is the affected version too far back to be back-ported into?

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1940811082)
I'm realising that tying these constants up to the keypool size would make it easier to reason about this test.

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1940815278)
Nice touch testing for persistence by loading it again!

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1940759007)
Yes, it can as per this comment in the issue: https://github.com/bitcoin/bitcoin/issues/29109#issuecomment-1863241394
The cpp code condition covers it as well though.

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1940933737)
```
'pubkey': '029edc63efe154f7f0da891f0def969b65cc4706893f8277580093eccbd09cb11d', 'ischange': True, 'timestamp': 1738665397, 'hdkeypath': "m/0'/0'/10'",
```

I found it a little weird that an external keychain address is categorised as change. I am assuming that internal keychain addresses are for changes.

💬 rkrux commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1940941450)
Nit: This change and the corresponding function call updates can be the first commit followed by the actual repairing. But I can understand if it's a bit much for this PR given its limited focus area.

💬 maflcko commented on pull request "ci: run in worktrees":
(https://github.com/bitcoin/bitcoin/pull/31787#discussion_r1940966104)
why is this needed after 6f5ae1a574562bf3fe938f704592c5374b43091a, which has this as a fallback?

💬 willcl-ark commented on pull request "ci: run in worktrees":
(https://github.com/bitcoin/bitcoin/pull/31787#discussion_r1940970290)
It is not needed since that is now merged

🤔 TheCharlatan reviewed a pull request: "Double check all block rules in `ConnectBlock`, not only `CheckBlock`"
(https://github.com/bitcoin/bitcoin/pull/31792#pullrequestreview-2592504179)
Concept ACK

💬 TheCharlatan commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1940976853)
In commit cddc0e2a0f2b8f19ef243edc8a85e206c31f9e62:
Can you add some context to the commit message for why this move is being done? Maybe:
"A call to ContextualCheckBlockHeader is added to ConnectBlock in the following commits, but it should not re-check the timestamp of the header there. Move the timestamp check into a separate function to avoid this."

💬 TheCharlatan commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1940977644)
Why is this not `LogError`?