💬 maflcko commented on pull request "test: check `scanning` field from `getwalletinfo`":
(https://github.com/bitcoin/bitcoin/pull/31768#discussion_r1940777811)
Won't this race with `importing` and intermittently fail?

💬 maflcko commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1940798789)
I'd be fine with a hacky check that only runs on windows and checks for a magic msvc binary string in `open(exe_path, "rb").read()`, but anything is fine and this is not a blocker from my side.

💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2633332880)
Rebased and addressed https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1925424138

💬 laanwj commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1940822375)
Unnecessary empty line removal

💬 laanwj commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1940823734)
Why delete this comment?

💬 laanwj commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1940835808)
Please add a descriptive comment what this code fragment does and why; we know, but the code isn't super easy to follow if you don't know the context.

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2633380099)
The ASan build fails with a long and cryptic error: https://github.com/bitcoin/bitcoin/actions/runs/13131433348/job/36637228850?pr=30975

```
/usr/bin/ld: CMakeFiles/mputil.dir/src/mp/util.cpp.o: in function `asan.module_ctor':
util.cpp:(.text.asan.module_ctor[asan.module_ctor]+0x6): undefined reference to `__asan_init'
/usr/bin/ld: util.cpp:(.text.asan.module_ctor[asan.module_ctor]+0xb): undefined reference to `__asan_version_mismatch_check_v8'
/usr/bin/ld: util.cpp:(.text.asan.module_cto
...

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2633384108)
macOS cross compile fails (in a different way): https://cirrus-ci.com/task/5052270577975296

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