🚀 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`?

💬 willcl-ark commented on pull request "ci: run in worktrees":
(https://github.com/bitcoin/bitcoin/pull/31787#discussion_r1940990613)
As far as I can tell, running `bash -c '( cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && RUST_BACKTRACE=1 cargo run )'` in a worktree already works fine for me, because the .git directory (referenced by the `.git` file that a worktree creates) is available to me, unlike in the containerised scenario where this path must be explicitly mounted into the container at runtime, which is what the helper script does.

I added some more comments to describe why the mount is needed in the s
...

💬 willcl-ark commented on pull request "ci: run in worktrees":
(https://github.com/bitcoin/bitcoin/pull/31787#discussion_r1940993491)
I don't think the file touch method will work for the same reasons we discussed above (needing the actual .git dir with all its objects and refs for tidy), so have not opted to go for that currently.

💬 maflcko commented on pull request "rpc: collect transaction fees on generateblock":
(https://github.com/bitcoin/bitcoin/pull/31775#issuecomment-2633641702)
The code doesn't pass existing tests, and doesn't have new tests. Also, the approach won't work anyway, because the transactions may not be in the mempool. You'd have to calculate the fees based on the prevout amounts for each transaction.

💬 maflcko commented on pull request "ci: run in worktrees":
(https://github.com/bitcoin/bitcoin/pull/31787#discussion_r1941030929)
```suggestion
docker run "${DOCKER_ARGS[@]}" bitcoin-linter
```

I don't think this env var will affect anything?

Also, style-wise I wonder if the contents of this file should be put into `ci/lint_run_all.sh`, so that there is only one bash script, instead of multiple.

Ideally there would only be one script, used by ci and locally, but that can be a follow-up. For now, just a single large if in `ci/lint_run_all.sh` should be fine to switch between the cirrus_ci code paths ( current con
...

💬 Sjors commented on pull request "test: added additional coverage to waitforblock and waitforblockheight rpc's":
(https://github.com/bitcoin/bitcoin/pull/31784#issuecomment-2633729812)
utACK 7e0db87d4fff996c086f6e86b62338c98ef30c55