💬 ajtowns commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1644977188)
I think `fPauseSend` getting set on both sides and causing a deadlock should probably be out of scope for this PR -- at least as I understand it, this fixes an issue where we get a deadlock even without `fPauseSend` triggering.

I think the scenario here is:

* peer A sends a 2MB message to peer B. This fills up B's socket receive buffer (200kB?) and A's socket send buffer (200kB?) without completing. A still has 1.6MB to send to B, so stops reading from the socket.
* peer B does the same
...

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1270283029)
(Just for context for the follow-up) I am not sure how "clean" the code in https://github.com/bitcoin/bitcoin/pull/28006 is. Maybe an easier alternative might be to remove all calls to `Get` and instead wrap the calls that acted on `FILE*` into a lambda (which acts on `FILE*`) and pass the lambda to an `Exec()` member function. The `Exec()` function would then also reset the cached `ftell` state and force a call to `ftell` the next time it is needed. On top of requiring that the constructor take
...

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1270296374)
Your concern is that the timeout will turn out not to be enough in some slow environments?

When you say "force the caller to sync" do you mean something like this:
```cpp
{
ASSERT_DEBUG_LOG("foo");
dostuff
somehow ensure the code that is supposed to log "foo" has executed
} // fail if "foo" is not in the log as soon as this is reached, like in `master`
```

?

💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1270336000)
Yes.

For example, if you are waiting for the scheduler to execute and log a validationinterface event, you can call `SyncWithValidationInterfaceQueue()` to sync without a timeout.

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1270350745)
You mean `SetMockTime(1610000000);`? Connman is already started at this point and I thought that the clock going backwards might be a problem, but replacing this with `SetMockTime(1610000000);` seems to work so I guess either one is fine. Do you think I should change it?

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