💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204480914)
03423f8bd12b95a06a4a9d8377e781625dd38aae This deleted comment line is still relevant when deleting the request:
```cpp
// Forget about all prior requests. If a block was already in-flight
// for a different peer, its BLOCKTXN response will be dropped.
```
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204480914)
03423f8bd12b95a06a4a9d8377e781625dd38aae This deleted comment line is still relevant when deleting the request:
```cpp
// Forget about all prior requests. If a block was already in-flight
// for a different peer, its BLOCKTXN response will be dropped.
```
💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204478988)
03423f8bd12b95a06a4a9d8377e781625dd38aae: some of the above deleted comment is still relevant.
```cpp
// If the peer does not send us a block, vBlocksInFlight remains non-empty,
// causing us to timeout and disconnect.
```
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204478988)
03423f8bd12b95a06a4a9d8377e781625dd38aae: some of the above deleted comment is still relevant.
```cpp
// If the peer does not send us a block, vBlocksInFlight remains non-empty,
// causing us to timeout and disconnect.
```
💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204497452)
03423f8bd12b95a06a4a9d8377e781625dd38aae could add a static assert `MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK > 1` in a followup.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204497452)
03423f8bd12b95a06a4a9d8377e781625dd38aae could add a static assert `MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK > 1` in a followup.
💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204491894)
03423f8bd12b95a06a4a9d8377e781625dd38aae: why not `return`?
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204491894)
03423f8bd12b95a06a4a9d8377e781625dd38aae: why not `return`?
💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204506528)
03423f8bd12b95a06a4a9d8377e781625dd38aae: "Give up" implies that this peer could have done something to prevent this, but that's often not the case (`Nodes sending cmpctblock messages SHOULD limit prefilledtxn to 10KB of transactions.` - BIP152 footnote 5).
```cpp
// Do not followup with `GETBLOCKTXN` to this peer,
// wait for an outbound peer to announce the compact block instead.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204506528)
03423f8bd12b95a06a4a9d8377e781625dd38aae: "Give up" implies that this peer could have done something to prevent this, but that's often not the case (`Nodes sending cmpctblock messages SHOULD limit prefilledtxn to 10KB of transactions.` - BIP152 footnote 5).
```cpp
// Do not followup with `GETBLOCKTXN` to this peer,
// wait for an outbound peer to announce the compact block instead.
💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204514939)
d7f359b35e8b1e9acc4d397de262cd9ba9bbcb83: maybe explain what's going on here. We send an unsolicited compact block from these peers to our node. Because the block adds to our tip the message doesn't get ignored. Once the block is successfully processed we mark the peer as high bandwidth. This is done on a first-come-first-serve basis. This results in the `[False, True, True, True]` pattern below.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204514939)
d7f359b35e8b1e9acc4d397de262cd9ba9bbcb83: maybe explain what's going on here. We send an unsolicited compact block from these peers to our node. Because the block adds to our tip the message doesn't get ignored. Once the block is successfully processed we mark the peer as high bandwidth. This is done on a first-come-first-serve basis. This results in the `[False, True, True, True]` pattern below.
💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204495961)
03423f8bd12b95a06a4a9d8377e781625dd38aae "on failure" -> "when missing transactions" (the word failure is ambiguous, this is different from `READ_STATUS_FAILED`)
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204495961)
03423f8bd12b95a06a4a9d8377e781625dd38aae "on failure" -> "when missing transactions" (the word failure is ambiguous, this is different from `READ_STATUS_FAILED`)
💬 denavila commented on pull request "Deniability - a tool to automatically improve coin ownership privacy":
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1561670353)
> This is really cool and I'd love to have it as an option in Bitcoin Core. However, most CoinJoin implementations are more advanced in this regard – for example, WabiSabi performs both divisions and consolidations. And it makes me wonder to which extent it's possible to decentralize coordination to the point where clients randomly take turns in doing it.
>
Yes, CoinJoin is definitely more powerful, however it has some drawbacks.
In particular, if it's possible to detect that an UTXO has
...
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1561670353)
> This is really cool and I'd love to have it as an option in Bitcoin Core. However, most CoinJoin implementations are more advanced in this regard – for example, WabiSabi performs both divisions and consolidations. And it makes me wonder to which extent it's possible to decentralize coordination to the point where clients randomly take turns in doing it.
>
Yes, CoinJoin is definitely more powerful, however it has some drawbacks.
In particular, if it's possible to detect that an UTXO has
...
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204554984)
Not anymore, we completely forget we expect it from a peer now. No disconnect should occur.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204554984)
Not anymore, we completely forget we expect it from a peer now. No disconnect should occur.
💬 Sjors commented on issue "Spurious (?) valgrind failure for p2p_compactblocks.py":
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1561678286)
I compiled on cce96182ba2457335868c65dc16b081c3dee32ee, but can reproduce on master. Updated description.
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1561678286)
I compiled on cce96182ba2457335868c65dc16b081c3dee32ee, but can reproduce on master. Updated description.
💬 denavila commented on pull request "Deniability - a tool to automatically improve coin ownership privacy":
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1561680006)
> It seems like a feature like this would be better to integrate into the wallet directly rather than something that is done through and managed by the GUI.
Do you mean moving some of the core logic inside the interfaces::Wallet API, or the CWallet implementation?
Yes, certainly, that could have benefits. I had to workaround certain things that weren't in the Wallet API (namely address reservation, bump fee handling etc). So that would clean up the wallet side for sure.
Since this was m
...
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1561680006)
> It seems like a feature like this would be better to integrate into the wallet directly rather than something that is done through and managed by the GUI.
Do you mean moving some of the core logic inside the interfaces::Wallet API, or the CWallet implementation?
Yes, certainly, that could have benefits. I had to workaround certain things that weren't in the Wallet API (namely address reservation, bump fee handling etc). So that would clean up the wallet side for sure.
Since this was m
...
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204558056)
if it's 1, that's fine, it just won't allow additional slots
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204558056)
if it's 1, that's fine, it just won't allow additional slots
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204558678)
oops, indeed, looks like we'll send off bogus getblocktxns here, if it comes from a preferred peer that's not first. needs a fix.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204558678)
oops, indeed, looks like we'll send off bogus getblocktxns here, if it comes from a preferred peer that's not first. needs a fix.
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204561198)
oops, indeed, looks like we'll send off bogus getblocktxns here, if it comes from a preferred peer that's not first. needs a fix. Wasted bandwidth and should result in "Peer %d sent us block transactions for block we weren't expecting\n"
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204561198)
oops, indeed, looks like we'll send off bogus getblocktxns here, if it comes from a preferred peer that's not first. needs a fix. Wasted bandwidth and should result in "Peer %d sent us block transactions for block we weren't expecting\n"
💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204563081)
That was already the case. The comment is there to point out that the current request will result in a disconnect (unless a new request is made on time).
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204563081)
That was already the case. The comment is there to point out that the current request will result in a disconnect (unless a new request is made on time).
💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1204567065)
> Is there a reason for the first commit?
It is because this is the only place where we set the chain timestamps to be in the past. In the form of
```
block_1_timestamp = genesis + 1
block_2_timestamp = median(genesis, block_1) + 1
block_3_timestamp = median(genesis, block_1, block_2) + 1
```
And in the wallet, we use the clock time to set the descriptors/keys creation time.
So, even when we create a wallet prior mining the chain, the wallet will always have a birth time after it. Wh
...
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1204567065)
> Is there a reason for the first commit?
It is because this is the only place where we set the chain timestamps to be in the past. In the form of
```
block_1_timestamp = genesis + 1
block_2_timestamp = median(genesis, block_1) + 1
block_3_timestamp = median(genesis, block_1, block_2) + 1
```
And in the wallet, we use the clock time to set the descriptors/keys creation time.
So, even when we create a wallet prior mining the chain, the wallet will always have a birth time after it. Wh
...
💬 instagibbs commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561694264)
Code review ACK https://github.com/bitcoin/bitcoin/pull/26969/commits/355bc6098a7373feb1d59f9651a79e1477d22243
reviewed with `git diff master --color-moved-ws=ignore-all-space --color-moved=dimmed-zebra`. very straight forward set of changes.
We have a small bug that you could do in a fixup commit as well perhaps: https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204491894
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561694264)
Code review ACK https://github.com/bitcoin/bitcoin/pull/26969/commits/355bc6098a7373feb1d59f9651a79e1477d22243
reviewed with `git diff master --color-moved-ws=ignore-all-space --color-moved=dimmed-zebra`. very straight forward set of changes.
We have a small bug that you could do in a fixup commit as well perhaps: https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204491894
💬 mzumsande commented on issue "Frequent "Timeout downloading block" with 24.1":
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1561696153)
> I did block the peer right after I posted the latest logs four days ago, and after that the block timeouts dropped to 2-3 per day, but like I mentioned in the bug report, since Core doesn't normally log the IP of disconnected incoming peers you pretty much have to enable debug=net logging to find the IP of the misbehaving peer. Which on this particular node is ~1GB of logs per 2-3 hours.
The reason why connections/disconnections aren't logged for inbound peers is that these events are remot
...
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1561696153)
> I did block the peer right after I posted the latest logs four days ago, and after that the block timeouts dropped to 2-3 per day, but like I mentioned in the bug report, since Core doesn't normally log the IP of disconnected incoming peers you pretty much have to enable debug=net logging to find the IP of the misbehaving peer. Which on this particular node is ~1GB of logs per 2-3 hours.
The reason why connections/disconnections aren't logged for inbound peers is that these events are remot
...
💬 brunoerg commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561696565)
> We have a small bug that you could do in a fixup commit as well perhaps: https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204491894
Gonna address it, thanks.
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561696565)
> We have a small bug that you could do in a fixup commit as well perhaps: https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204491894
Gonna address it, thanks.
💬 instagibbs commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561703430)
Upon further consultation by @fanquake , I've been advised to just PR the change straight. keep this PR as is.
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561703430)
Upon further consultation by @fanquake , I've been advised to just PR the change straight. keep this PR as is.