💬 darosior commented on issue "fuzz: miniscript: test the node is satisfiable before dereferencing `GetOps`":
(https://github.com/bitcoin/bitcoin/issues/28114#issuecomment-1645192031)
Okay, TIL. Or rather today i read the doc. Dereferencing an optional is [undefined behaviour](https://en.cppreference.com/w/cpp/utility/optional/operator*) (won't necessarily crash) whereas calling [`value()` would throw `std::bad_optional_access`](https://en.cppreference.com/w/cpp/utility/optional/value).

Simply changing one for the other will trigger the expected crash when running over the `qa-assets` corpus:
```diff
diff --git a/src/test/fuzz/miniscript.cpp b/src/test/fuzz/miniscript.cp
...

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1270388875)
Thinking about this - when one thread triggers an event which another thread is supposed to pick and do something about it and the first thread wants to confirm that was done, then some waiting must be involved in the first thread.

`SyncWithValidationInterfaceQueue()` uses https://en.cppreference.com/w/cpp/thread/future/wait which is unlimited, without a timeout. In practice I guess that means a 2h (?) timeout until some guard in the CI kills the waiting test.

Syncing in the test, explicit
...

💬 MarcoFalke commented on issue "fuzz: miniscript: test the node is satisfiable before dereferencing `GetOps`":
(https://github.com/bitcoin/bitcoin/issues/28114#issuecomment-1645205300)
I think there is a check for both dereferences of the optional?

```cpp
if (provider.ConsumeBool() && node_ops && *node_ops < MAX...
^^check ^^deref

💬 darosior commented on issue "fuzz: miniscript: test the node is satisfiable before dereferencing `GetOps`":
(https://github.com/bitcoin/bitcoin/issues/28114#issuecomment-1645209091)
:facepalm:

💬 darosior commented on issue "fuzz: miniscript: test the node is satisfiable before dereferencing `GetOps`":
(https://github.com/bitcoin/bitcoin/issues/28114#issuecomment-1645212277)
Next time i'll make sure to have coffee before commenting on anything. Sorry for the noise (again).

✅ darosior closed an issue: "fuzz: miniscript: test the node is satisfiable before dereferencing `GetOps`"
(https://github.com/bitcoin/bitcoin/issues/28114)

💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1270402838)
Not sure about an infinite timeout for `ASSERT_DEBUG_LOG`. For example, this can easily be hit when the log string is modified by a developer, turning a (doc) change in one place into a hard to debug unit test issue.

I know that it is possible to hit an infinite runtime in any other place, but that would generally mean a major fault or injected bug elsewhere. For example, if the scheduler thread is stopped and one called `SyncWithValidationInterfaceQueue()`, it would be quite obvious from the
...

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1645220283)
re-ACK 33a2f6dc3c2512129df7882586acb3b3ddce62cb

💬 MarcoFalke commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#issuecomment-1645228316)
Needs rebase if still relevant

💬 MarcoFalke commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1270411973)
https://cirrus-ci.com/task/6496120092753920?logs=ci#L3132

```
test 2023-07-17T05:13:02.641000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "/tmp/cir
...

💬 MarcoFalke commented on pull request "RPC: Add universal options argument to listtransactions":
(https://github.com/bitcoin/bitcoin/pull/22807#issuecomment-1645232141)
Maybe mark as draft/WIP for as long as this is still WIP?

💬 MarcoFalke commented on pull request "descriptors: Add a KEY expression representing a list of individual keys":
(https://github.com/bitcoin/bitcoin/pull/26626#issuecomment-1645233520)
Maybe mark as draft for as long as CI is red?

💬 MarcoFalke commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1645233777)
Maybe mark as draft for as long as CI is red?

💬 fanquake commented on pull request "descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1645240967)
Backported 2/3 of the commits here in #https://github.com/bitcoin/bitcoin/pull/28067.

💬 darosior commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1270423929)
Updated the comment.

💬 fanquake commented on pull request "depends: xcb-proto 1.15.2":
(https://github.com/bitcoin/bitcoin/pull/28097#issuecomment-1645248859)
Backported this to 25.x in #28047, so that we'll avoid build failures with new Python. Already an issue for anyone building the current release branch on rawhide or similar.

💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1270441940)
> Do you think I should change it?

In any case, there should be a comment to explain why it sets the mock time, no?

💬 darosior commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1270444558)
Done.

📝 kristapsk converted_to_draft a pull request: "RPC: Add universal options argument to listtransactions"
(https://github.com/bitcoin/bitcoin/pull/22807)
Taken from #19443. There two additional arguments `paginatebypointer` and `nextpagepointer` were added and @luke-jr proposed that they could be united with existing `skip` argument into single `options` JSON argument. This is better approach than continue adding more and more different arguments for different output options. #22775 proposes to add sorting order as additional argument, I think it's better to use the same approach there too. Having this as a separate PR will make reviews and testi
...

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1270498856)
> Not sure about an infinite timeout for `ASSERT_DEBUG_LOG`. For example, this can easily be hit when the log string is modified by a developer, turning a (doc) change in one place into a hard to debug unit test issue.

I agree. I will stick to a meaningful message after a timeout instead of infinite wait (without a message).

> I know that it is possible to hit an infinite runtime in any other place, but that would generally mean a major fault or injected bug elsewhere. For example, if the
...