💬 fjahr commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1758617119)
My thought here was that we set a higher factor when things are generally expected to run slower, so in that case it seems natural to me that we will also want to wait longer. But yeah, I didn't take into account that people will set this factor unreasonably high to not see timeouts at all. I have removed it for now but I think an alternative could be to set a reasonable upper bound like 10x.

💬 fjahr commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1758617767)
Done and renamed to `duration`.

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2348604652)
The first two commits of https://github.com/Sjors/bitcoin/pull/62 could be included here to put it behind a `-DWITH_SV2=ON` feature flag.

I'll include that in a future push once it had some review. It shouldn't be a blocker for this PR, because the classes introduced here are not used yet.

💬 fjahr commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1758618169)
sounds good, done

💬 fanquake commented on pull request "build: add `standard branch-protection` to hardening flags for aarch64-linux":
(https://github.com/bitcoin/bitcoin/pull/30433#issuecomment-2348605539)
Sorry, fixed the buggy previous version.

✅ hebasto closed an issue: "Closing a wallet using the fa46088440 28.x QT client segfaults"
(https://github.com/bitcoin/bitcoin/issues/30887)

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