🤔 jonatack reviewed a pull request: "doc: cmake: prepend "build" to functional/test_runner.py"
(https://github.com/bitcoin/bitcoin/pull/30859#pullrequestreview-2303553463)
Concept ACK. I came across this as well, and the new error message would indeed be helpful. (Bonus points if the message can be defined in one place, didn't check.)

💬 jonatack commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1759154738)
```suggestion
raise Exception(f"config.ini file {self.options.configfile} not found, be sure to run this script from within the build directory")
```

💬 jonatack commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1759154535)
```suggestion
raise Exception(f"config.ini file {configfile} not found, be sure to run this script from within the build directory")
```

🤔 danielabrozzoni reviewed a pull request: "signet: fixing mining for OP_TRUE challenge"
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2303581034)
Concept ACK

> However contrib/signet/miner can't handle this, as it fails with PSBT signing failed.

I can't seem to have contrib/signet/miner fail on master, am I doing something wrong?
I'm on 1d5b2406bb9ce619219a3b76608bd764a3b162c3,
I'm testing manually with `bitcoind -signet -signetchallenge=51` and:
```
❯ $MINER --cli="$CLI" generate --grind-cmd="$GRIND" --address="$ADDR" --nbits=$NBITS --ongoing
2024-09-13 18:29:59 INFO Mined block at height 24; next in -30h15m33s (mine)
2024-09
...

💬 danielabrozzoni commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1759171230)
I would rephrase as "the walletprocesspsbt step *has* to be skipped", as it currently returns an error:

```
❯ $CLI -signet getblocktemplate '{"rules": ["signet","segwit"]}' | $MINER --cli="$CLI" genpsbt --address="$ADDR" | $CLI -signet -stdin walletprocesspsbt
error code: -22
error message:
Specified sighash value does not match value stored in PSBT
```

🤔 jonatack reviewed a pull request: "kernel: Move background load thread to node context"
(https://github.com/bitcoin/bitcoin/pull/30896#pullrequestreview-2303608061)
Light ACK bc7900f33db3d01fb93dfee7981c01ea495cd42e

Good further separation of concerns.

💬 ryanofsky commented on pull request "Fix crash when closing wallet":
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2349397769)
Code review ACK a965f2bc07a3588f8c2b8d6a542961562e3f5d0e

This seems like a workable fix, but I think it it is not ideal. The underlying problem here is that https://github.com/bitcoin/bitcoin/pull/30659 moved `NotifyUnload` from `FlushAndDeleteWallet` to `RemoveWallet`, so now instead of `NotifyUnload` guaranteed being called once, it can now be called multiple times, but the GUI code assumes it is only called once and double deletes the wallet model and crashes. I think a better for this is
...

💬 pablomartin4btc commented on pull request "Fix crash when closing wallet":
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2349418124)
> now instead of NotifyUnload guaranteed being called once, it can now be called multiple times, but the GUI code assumes it is only called once and double deletes the wallet model and crashes.

How can I repro the crash?

💬 ryanofsky commented on pull request "Fix crash when closing wallet":
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2349438972)
> How can I repro the crash?

I don't know of an easy way to reproduce the crash after the current fix, but it is possible to revert the current and test the alternate suggest fix by using the same steps in the PR description, choosing close wallet from the GUI menu.

It might also be possible to make the GUI after the current fix by enabling RPC and calling unloadwallet RPC simultaneously from multiple threads, but not sure about that.

👍 stickies-v approved a pull request: "kernel: Move background load thread to node context"
(https://github.com/bitcoin/bitcoin/pull/30896#pullrequestreview-2303572995)
ACK 3a26c839c63ecda2d3b2491d6d84a03c1d8f508f

I looked into the history of why `m_thread_load` is a `ChainstateManager` member, but it appears there's no particular reason. faf843c07f99f91603e08ea858f972516f1d669a deglobalized `g_load_block` into `ChainstateManager::m_load_block` (later renamed (04575106b2529f495ce8110ddf7ed2247d4bc339) to `m_thread_load`) and it appears the clean-up of this PR could have been applied to that commit too (but building that >3yo commit seems non-trivial so I can
...

💬 stickies-v commented on pull request "kernel: Move background load thread to node context":
(https://github.com/bitcoin/bitcoin/pull/30896#discussion_r1759167497)
nit: needs Doxygen cross-reference update in `developer-notes.md` to reflect the new anchor `structnode_1_1_node_context.html#aef02af873199206a8f032a3917277bef` (as per `cmake --build build -t docs` on 3a26c839c63ecda2d3b2491d6d84a03c1d8f508f)

<details>
<summary>git diff on 3a26c839c6</summary>

```diff
diff --git a/doc/developer-notes.md b/doc/developer-notes.md
index a630957f41..febd158c68 100644
--- a/doc/developer-notes.md
+++ b/doc/developer-notes.md
@@ -634,7 +634,7 @@ Threads

...

💬 brunoerg commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2349471125)
> I briefly fuzzed after making the change and didn't hit anything. Likely I just didn't fuzz enough, but posting just in case somebody has a fuzz output that I could use to verify.

I left different mutations of it running overnight (12h+) and I also didn't get a crash for this one.

> I think it would be better not to add anything like the above because one might fuzz bitcoind directly, e.g. we could consider dropping the custom honggfuzz netdriver patch and use FUZZING_BUILD_MODE_UNSAFE_F
...

💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2349476460)
> I briefly fuzzed after making the change and didn't hit anything. Likely I just didn't fuzz enough, but posting just in case somebody has a fuzz output that I could use to verify.

@glozow thanks for reviewing. I'm looking into this more. Good catch actually. Turns out I only partially reverted that commit when testing (only moved the `return true` out of the `else` in `TryLowWorkHeadersSync`). In that case, the fuzzer crashes on this line:
https://github.com/bitcoin/bitcoin/blob/e43ce250c6
...

💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2349486479)
> I left different mutations of it running overnight (12h+) and I also didn't get a crash for this one.

Good to know, thanks. I'll mark this PR as a draft for now until I figure what's going on.

📝 marcofleon converted_to_draft a pull request: "fuzz: Test headers pre-sync through p2p"
(https://github.com/bitcoin/bitcoin/pull/30661)
This PR reopens https://github.com/bitcoin/bitcoin/pull/28043. It's a regression fuzz test for https://github.com/bitcoin/bitcoin/pull/26355 and [a couple bugs](https://github.com/bitcoin/bitcoin/pull/25717/commits/ed6cddd98e32263fc116a4380af6d66da20da990) that were addressed in https://github.com/bitcoin/bitcoin/pull/25717. This should help us move forward with the [removal of mainnet checkpoints](https://github.com/bitcoin/bitcoin/pull/25725).

It seems like the main concern in https://githu
...

👍 instagibbs approved a pull request: "cluster mempool: optimized candidate search"
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2303655329)
reACK 9ad2fe7e69e9e69949ebbb280a15756dc3301f09

just a rebase on master to take up `Revert "build: Minimize I/O operations in `GenerateHeaderFrom{Json,Raw}.cmake`"` I presume

via `git range-diff master a5ea6467b6a694facc1f5efe460bf30539167597 9ad2fe7e69e9e69949ebbb280a15756dc3301f09`

⚠️ 1440000bytes opened an issue: "Use IPv4-encoded IPv6 address to get IPv4 node address with port number from DNS seeds"
(https://github.com/bitcoin/bitcoin/issues/30900)
### Please describe the feature you'd like to see added.

Use the IPv4-encoded IPv6 addresses that start with a reserved prefix to support nodes with non-default ports in bootstrapping process.

There are 7k IPv4 reachable nodes and some of them use non-default ports:


Example: Port 39388 is used by more than [1000 nodes](https://bitnodes.io/nodes/all/ports/)

The scripts used in this testing guide
...

💬 Sjors commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1759245113)
> sighash value does not match

That's a bug I think, but I'm not sure what's causing it.

🤔 jonatack reviewed a pull request: "doc: fixed inconsistencies in documentation between autotools to cmake change"
(https://github.com/bitcoin/bitcoin/pull/30875#pullrequestreview-2303730050)
Concept ACK

💬 jonatack commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1759264704)
```suggestion
was in the active chain, as would be the case after calling the `invalidateblock` RPC.
```