💬 fanquake commented on pull request "[28.x] Further backports and rc2":
(https://github.com/bitcoin/bitcoin/pull/30827#issuecomment-2349321778)
> Feel free to pick [this](https://github.com/hebasto/bitcoin/commit/be6354db410d162f82b5e3c76d964c0365e51d96) translation update

There's not so much of a rush here that this should bypass the normal PR & review process. This should first be PR'd and merged into master and then backported. Otherwise any broken string are still in master. There is at least one other change waiting for 28.x, so rc2 also needs to either wait for that, or we'll be having an rc3 (which can include the ts update) i
...

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2349326264)
@sdaftuar is that number of iterations per search invocation, or total over the entire linearization?

Could a version of this graph show the total fraction of *transactions* that are optimally linearized? Or maybe just do a chart where a cluster min size of <larger number> is used to ignore the many small clusters we know we can optimally linearize?

💬 hebasto commented on pull request "[28.x] Further backports and rc2":
(https://github.com/bitcoin/bitcoin/pull/30827#issuecomment-2349332544)
> There's not so much of a rush here that this should bypass the normal PR & review process.

Sure.

> This should first be PR'd and merged into master and then backported.

After branching off, the Transifex.com has translation for the version branches only. Fetching 28x translations into the master branch does not look reasonable.

> Otherwise any broken string are still in master.

Maybe this case is a special one.

💬 rkrux commented on issue "28.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2349335396)
@fjahr I've updated the guide adding instructions for assumeutxo mainnet.

📝 hebasto opened a pull request: "qt: Translations update"
(https://github.com/bitcoin/bitcoin/pull/30899)
The recent translations from Transifex.com fetched with the bitcoin-maintainer-tools/update-translations.py tool.

Fixes https://github.com/bitcoin/bitcoin/issues/30897.

💬 hebasto commented on pull request "[28.x] Further backports and rc2":
(https://github.com/bitcoin/bitcoin/pull/30827#issuecomment-2349337355)
> > There's not so much of a rush here that this should bypass the normal PR & review process.
>
> Sure.

Please see https://github.com/bitcoin/bitcoin/pull/30899.

🤔 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.