💬 ishaanam commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#issuecomment-1644753444)
The commit message for 68bb1463dc1f995fbfdb75d3acef625bde104275 should be updated because now that #27145 has gotten merged, the described issue has already been fixed.
(https://github.com/bitcoin/bitcoin/pull/27286#issuecomment-1644753444)
The commit message for 68bb1463dc1f995fbfdb75d3acef625bde104275 should be updated because now that #27145 has gotten merged, the described issue has already been fixed.
💬 ajtowns commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270121452)
Not if you make it `WantsToSend() EXCLUSIVE_LOCK_REQUIRED(cs_vSend)` and require the caller to have the lock?
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270121452)
Not if you make it `WantsToSend() EXCLUSIVE_LOCK_REQUIRED(cs_vSend)` and require the caller to have the lock?
💬 ajtowns commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270122517)
Err, doesn't that mean this behaved the same as the previous code? How did it [fix anything](https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1608257369)?
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1270122517)
Err, doesn't that mean this behaved the same as the previous code? How did it [fix anything](https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1608257369)?
💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270122616)
Done, I've replaced `UpdateTag` with `ComputeTag`, which does the whole tag calculation, including poly1305 key generation.
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270122616)
Done, I've replaced `UpdateTag` with `ComputeTag`, which does the whole tag calculation, including poly1305 key generation.
💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270123417)
I believe it's better not to do that, though it really doesn't matter much.
The reason is that if we'd start by generating the key, we'd have to store that key in memory somewhere, leave it there for the whole encryption, then fetch it again (at which point it's quite possibly gone from CPU caches) to compute the tag. By seeking and deriving at the end, we only need the chacha20 key/state, which is likely still hot at that point (as it's needed every 64 bytes of encryption). And the seeking i
...
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270123417)
I believe it's better not to do that, though it really doesn't matter much.
The reason is that if we'd start by generating the key, we'd have to store that key in memory somewhere, leave it there for the whole encryption, then fetch it again (at which point it's quite possibly gone from CPU caches) to compute the tag. By seeking and deriving at the end, we only need the chacha20 key/state, which is likely still hot at that point (as it's needed every 64 bytes of encryption). And the seeking i
...
💬 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
...