🚀 hebasto merged a pull request: "Fix crash when closing wallet"
(https://github.com/bitcoin-core/gui/pull/835)

💬 fjahr commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#issuecomment-2348606489)
Addressed feedback from @maflcko and @jonatack and fixed the missing `lambda:` causing the CI failure.

💬 hebasto commented on pull request "[28.x] Further backports and rc2":
(https://github.com/bitcoin/bitcoin/pull/30827#issuecomment-2348608460)
@achow101

Could you please add https://github.com/bitcoin-core/gui/pull/835 as well?

💬 maflcko commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1758629596)
```suggestion
def ensure_helper_internal(predicate, *, duration):
```

forgot to rename this one?

💬 maflcko commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1758629070)
```suggestion
def ensure(self, test_function, *, **kwargs):
```

I am not sure about a default value here. I feel like `time.sleep()`, or similar constructs in tests should be easily visible. Otherwise, this may be used as a drop-in replacement for `assert`, because someone may think this is just an assert that accepts a lambda.

💬 fjahr commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1758637789)
I didn't think it matters much because this is internal anyway, but I renamed it now as well.

💬 fjahr commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1758642035)
Ok, that might be more of a naming issue with `ensure` then which I am happy to reconsider if someone has a better idea but I removed the default value for now.

👍 tdb3 approved a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2302812511)
ACK b9aded42252969cc81acf4122ca5e2d1e00ecf90

💬 dergoegge commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1758657903)
> There is also an overhead that we probably don't want a separate fuzz target for every trivial function, because each fuzz target may be put into a separate binary and is allocated a separate folder.

I don't think the binary sizes would be a problem after we merge something like https://github.com/bitcoin/bitcoin/pull/30882 and the extra folder for the corpus is also not a huge cost (they compress really nicely).

I'd prefer to follow best practices over having to weigh triviality against
...

💬 sipa commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758661956)
I believe that when `ignore` is used, it's usually for small amounts of data, which are likely already cached.

💬 sipa commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758665750)
I tried that and it caused test failures. My reasoning is that the approach here is fine, because a failure to tell what position a `FILE*` is at means that further operations will fail too, anyway.

👍 tdb3 approved a pull request: "doc: unit test runner help fixup"
(https://github.com/bitcoin/bitcoin/pull/30890#pullrequestreview-2302846500)
ACK 0024d2c6ea0fafe9b9949af4bbcd0c583e580746

Useful doc improvements, thanks.

💬 fanquake commented on pull request "doc: replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1758683043)
A few lines above here. This
```
cd <BITCOIN_SOURCE_DIRECTORY>
make -C depends NO_QT=1 MULTIPROCESS=1
CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site ./configure
make
src/bitcoin-node -regtest -printtoconsole -debug=ipc
BITCOIND=bitcoin-node test/functional/test_runner.py
```

should be replaced with:
```bash
cd <BITCOIN_SOURCE_DIRECTORY>
make -C depends NO_QT=1 MULTIPROCESS=1
cmake -B build --toolchain=depends/x86_64-pc-linux-gnu/toolchain.cmake
cmake --build build
...

💬 Zk2u commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2348699534)
You're a legend - thank you.

🤔 pablomartin4btc reviewed a pull request: "log: Use ConstevalFormatString"
(https://github.com/bitcoin/bitcoin/pull/30889#pullrequestreview-2302866708)
ACK fabd78e2e30e557e04739e29c9c221ad47245df1

I find this very practical. Regarding the abort on CI specifically would be very useful as well in order to catch the issues earlier, not sure about the cons on test-only builds, perhaps someone can add some issues related with it.

📝 TheCharlatan opened a pull request: "kernel: Move background load thread to node context"
(https://github.com/bitcoin/bitcoin/pull/30896)
The thread handle is never used by the ChainstateManager, so move it out and into the node context. Users of the kernel library now no longer have to manually join the thread when destructing the ChainstateManager.

💬 sipa commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758696378)
Fixed.

🤔 tdb3 reviewed a pull request: "doc: replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes"
(https://github.com/bitcoin/bitcoin/pull/30875#pullrequestreview-2302866367)
Approach ACK

💬 tdb3 commented on pull request "doc: replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1758692937)
nit: IMHO, could get rid of the "As of 0.12" part, since we're using CMake now and and 0.12 didn't use it.

💬 tdb3 commented on pull request "doc: replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1758698079)
nit: "configure script" is an Autotools thing, right?