💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270123471)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270123471)
Fixed.
💬 sipa commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270125479)
@ajtowns The old code here first added a SEND, and then a RECEIVE, so if both were performed, only a SEND would be present in the map, meaning an unconditional send.
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270125479)
@ajtowns The old code here first added a SEND, and then a RECEIVE, so if both were performed, only a SEND would be present in the map, meaning an unconditional send.
💬 mzumsande commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270127032)
I could reproduce the problem in a functional test today - for me, neither version (including the current one) fixes the problem completely. Will post detailed results soon.
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270127032)
I could reproduce the problem in a functional test today - for me, neither version (including the current one) fixes the problem completely. Will post detailed results soon.
💬 mzumsande commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1644864253)
I wrote a functional test, see https://github.com/mzumsande/bitcoin/tree/test_sipa_netstalling (because of the 1st commit obviously not intended for merge, but it makes it possible to reproduce the problem).
It works by mining a few large blocks, and having two nodes exchange these blocks in both direction by repeated `getblockfrompeer` messages, and then check whether the deadlock happened.
Unfortunately, the current branch doesn't appear to fix the problem completely, the test fails for
...
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1644864253)
I wrote a functional test, see https://github.com/mzumsande/bitcoin/tree/test_sipa_netstalling (because of the 1st commit obviously not intended for merge, but it makes it possible to reproduce the problem).
It works by mining a few large blocks, and having two nodes exchange these blocks in both direction by repeated `getblockfrompeer` messages, and then check whether the deadlock happened.
Unfortunately, the current branch doesn't appear to fix the problem completely, the test fails for
...
💬 jonatack commented on pull request "test: update tool_wallet coverage for unexpected writes to wallet":
(https://github.com/bitcoin/bitcoin/pull/28116#issuecomment-1644870154)
Pulled in 2 commits from #27850 by @pinheadmz that this is now based on. Will rebase once that PR is merged.
(https://github.com/bitcoin/bitcoin/pull/28116#issuecomment-1644870154)
Pulled in 2 commits from #27850 by @pinheadmz that this is now based on. Will rebase once that PR is merged.
💬 ariard commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1270139854)
A comment can be added to precise this is the global not per-peer limit, at least on how it is used by `LimitOrphans()` (`DEFAULT_MAX_ORPHAN_TRANSACTIONS`).
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1270139854)
A comment can be added to precise this is the global not per-peer limit, at least on how it is used by `LimitOrphans()` (`DEFAULT_MAX_ORPHAN_TRANSACTIONS`).
💬 ajtowns commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270175120)
> @ajtowns The old code here first added a SEND, and then a RECEIVE, so if both were performed, only a SEND would be present in the map, meaning an unconditional send.
Right, but master does the same thing, only emplacing SEND whenever vSendMsg isn't empty; so seems weird that the original patch here was observed to help anything?
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270175120)
> @ajtowns The old code here first added a SEND, and then a RECEIVE, so if both were performed, only a SEND would be present in the map, meaning an unconditional send.
Right, but master does the same thing, only emplacing SEND whenever vSendMsg isn't empty; so seems weird that the original patch here was observed to help anything?
💬 sipa commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270177198)
@ajtowns You're right. I'm confused now. @mzumsande's observation that stalling can also be caused by fPauseRecv being set on both sides seems like a partial explanation, but not the whole thing.
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270177198)
@ajtowns You're right. I'm confused now. @mzumsande's observation that stalling can also be caused by fPauseRecv being set on both sides seems like a partial explanation, but not the whole thing.
💬 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
...
(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
...
(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`
```
?
(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.
(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?
(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
...
(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
...
(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
(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:
(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).
(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)
(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
...
(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
...