🤔 mzumsande reviewed a pull request: "Parallel compact block downloads, take 3"
(https://github.com/bitcoin/bitcoin/pull/27626#pullrequestreview-1440297516)
Code Review ACK d7f359b35e8b1e9acc4d397de262cd9ba9bbcb83
I've run this for >2 days now with additional logging and encountered no problems.
One thing I frequently observe that hasn't been mentioned above is that a low-bandwidth peer is first to give us a new header, so there is a header/getdata(CMPCT) roundtrip, and by the time that has finished we already requested the block additionally from a high-bandwidth peer that doesn't need the roundtrip.
(https://github.com/bitcoin/bitcoin/pull/27626#pullrequestreview-1440297516)
Code Review ACK d7f359b35e8b1e9acc4d397de262cd9ba9bbcb83
I've run this for >2 days now with additional logging and encountered no problems.
One thing I frequently observe that hasn't been mentioned above is that a low-bandwidth peer is first to give us a new header, so there is a header/getdata(CMPCT) roundtrip, and by the time that has finished we already requested the block additionally from a high-bandwidth peer that doesn't need the roundtrip.
💬 mzumsande commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202794974)
Another nit about the `Assume(mapBlocksInFlight.count(hash) <= MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK);` line - I think that this should this be `<` instead of `<=` because it is the responsibility of the caller to call `BlockRequested` only if there is room for one more request.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202794974)
Another nit about the `Assume(mapBlocksInFlight.count(hash) <= MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK);` line - I think that this should this be `<` instead of `<=` because it is the responsibility of the caller to call `BlockRequested` only if there is room for one more request.
💬 jamesob commented on pull request "validation: have LoadBlockIndex account for snapshot use":
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202808054)
But it's no problem for them to have the ASSUMED_VALID bit set to true; since they're beneath the snapshot base height, they'll be added to the setBlockIndexCandidates.
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202808054)
But it's no problem for them to have the ASSUMED_VALID bit set to true; since they're beneath the snapshot base height, they'll be added to the setBlockIndexCandidates.
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202811629)
caller may call it when it's already requested by this peer, see a few lines below at https://github.com/bitcoin/bitcoin/pull/27626/files/13f9b20b4cb2f3f26e81184a77e9cf1f626d4f57..03423f8bd12b95a06a4a9d8377e781625dd38aae#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3L1175
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202811629)
caller may call it when it's already requested by this peer, see a few lines below at https://github.com/bitcoin/bitcoin/pull/27626/files/13f9b20b4cb2f3f26e81184a77e9cf1f626d4f57..03423f8bd12b95a06a4a9d8377e781625dd38aae#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3L1175
💬 sr-gi commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1559935656)
What's the rationale regarding optimizing for memory instead of privacy here? I can see how this simplifies the design, however, I fail to see this being an obvious pick given we'd be potentially even more exposed to fingerprinting than before (as is now it is trivial for every connection to probe data on the node's mempool).
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1559935656)
What's the rationale regarding optimizing for memory instead of privacy here? I can see how this simplifies the design, however, I fail to see this being an obvious pick given we'd be potentially even more exposed to fingerprinting than before (as is now it is trivial for every connection to probe data on the node's mempool).
💬 sdaftuar commented on pull request "validation: have LoadBlockIndex account for snapshot use":
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202821267)
The code here isn't comparing with the snapshot base height; it's just checking if a block index entry is below the `first_assumed_valid_height`, and only in that case does it get added to `setBlockIndexCandidates`. [I don't believe there's any other logic elsewhere that will do what you are describing, of adding entries below the snapshot base height to `setBlockIndexCandidates`?]
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202821267)
The code here isn't comparing with the snapshot base height; it's just checking if a block index entry is below the `first_assumed_valid_height`, and only in that case does it get added to `setBlockIndexCandidates`. [I don't believe there's any other logic elsewhere that will do what you are describing, of adding entries below the snapshot base height to `setBlockIndexCandidates`?]
💬 sdaftuar commented on pull request "validation: have LoadBlockIndex account for snapshot use":
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202825049)
More generally: I think that we don't need to worry at all about whether the assumed-valid bit is set when adding entries to `setBlockIndexCandidates`. We always validate a block when it gets connected, so the only invariant we need on `setBlockIndexCandidates` is whether they have enough work to be worth considering. (We used to have a hard requirement that we always HAVE_DATA for such entries, but after pruning we had to relax that a bit, as there are edge-case scenarios where you might HAVE_
...
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202825049)
More generally: I think that we don't need to worry at all about whether the assumed-valid bit is set when adding entries to `setBlockIndexCandidates`. We always validate a block when it gets connected, so the only invariant we need on `setBlockIndexCandidates` is whether they have enough work to be worth considering. (We used to have a hard requirement that we always HAVE_DATA for such entries, but after pruning we had to relax that a bit, as there are edge-case scenarios where you might HAVE_
...
💬 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_r1202827015)
To save some extra processing, because they are performed at different levels.
At the spkm level (here), we are at the lowest one. So, before emitting an event that will require to be processed by all the listeners (the wallets and/or other registered objects), we can just check if the updated descriptor timestamp is lower than the existent one.
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1202827015)
To save some extra processing, because they are performed at different levels.
At the spkm level (here), we are at the lowest one. So, before emitting an event that will require to be processed by all the listeners (the wallets and/or other registered objects), we can just check if the updated descriptor timestamp is lower than the existent one.
💬 mzumsande commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202830069)
Hmm, is the case where we are at `MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK` but `BlockRequested()` is still called one something that can actually happen? Because in that case that caller would need to have previously checked that one of the existing requests is for the same nodeId, otherwise the limit would be breached.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202830069)
Hmm, is the case where we are at `MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK` but `BlockRequested()` is still called one something that can actually happen? Because in that case that caller would need to have previously checked that one of the existing requests is for the same nodeId, otherwise the limit would be breached.
💬 jamesob commented on pull request "validation: have LoadBlockIndex account for snapshot use":
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202835874)
> The code here isn't comparing with the snapshot base height; it's just checking if a block index entry is below the first_assumed_valid_height
Oops, total misread on my part. You're right; `first_assumed_valid_height` will be the block after the tip of the background chain.
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202835874)
> The code here isn't comparing with the snapshot base height; it's just checking if a block index entry is below the first_assumed_valid_height
Oops, total misread on my part. You're right; `first_assumed_valid_height` will be the block after the tip of the background chain.
💬 ryanofsky commented on pull request "validation: have LoadBlockIndex account for snapshot use":
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202842452)
The logic here does seem confusing.
Code comment says background candidate set needs to exclude BLOCK_ASSUMED_VALID blocks because otherwise ActivateBestChain "would add assumed-valid blocks to the chain". But that seems like exactly what it should do? (after validating them)
Commit message says background candidate set needs to exclude blocks after the snapshot height that don't have BLOCK_ASSUMED_VALID, but depend on blocks that do have BLOCK_ASSUMED_VALID. That idea does make sense, but
...
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202842452)
The logic here does seem confusing.
Code comment says background candidate set needs to exclude BLOCK_ASSUMED_VALID blocks because otherwise ActivateBestChain "would add assumed-valid blocks to the chain". But that seems like exactly what it should do? (after validating them)
Commit message says background candidate set needs to exclude blocks after the snapshot height that don't have BLOCK_ASSUMED_VALID, but depend on blocks that do have BLOCK_ASSUMED_VALID. That idea does make sense, but
...
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202844909)
make sure to check the other call locations, but they aren't supposed to
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202844909)
make sure to check the other call locations, but they aren't supposed to
💬 achow101 commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1560024143)
ACK 272eb5561667482f8226bcf98eea00689dccefb8
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1560024143)
ACK 272eb5561667482f8226bcf98eea00689dccefb8
✅ achow101 closed an issue: "failure in feature_bip68_sequence.py"
(https://github.com/bitcoin/bitcoin/issues/27129)
(https://github.com/bitcoin/bitcoin/issues/27129)
🚀 achow101 merged a pull request: "test: fix intermittent issue in `feature_bip68_sequence`"
(https://github.com/bitcoin/bitcoin/pull/27177)
(https://github.com/bitcoin/bitcoin/pull/27177)
💬 jamesob commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1560035238)
New bug, albeit pretty minor:
- [ ] if you restart the node after activating the snapshot but before connecting any blocks to the snapshot chainstate, `ReadBlockFromDisk` checks fail when calling `VerifyDB()` during chainstate initialization. I'm not sure when this would happen in practice, but it's an annoying edge case.
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1560035238)
New bug, albeit pretty minor:
- [ ] if you restart the node after activating the snapshot but before connecting any blocks to the snapshot chainstate, `ReadBlockFromDisk` checks fail when calling `VerifyDB()` during chainstate initialization. I'm not sure when this would happen in practice, but it's an annoying edge case.
💬 schjonhaug commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1560053023)
Looks like `>>` is missing after `"maxconnections=2"` in
```
$ cd $DATA_DIR && echo "signet=1" >> bitcoin.conf && echo "maxconnections=2" bitcoin.conf && cd ~
```
Also, it’s easier to copy the commands using the (hover) copy icon <img width="46" alt="image" src="https://github.com/bitcoin/bitcoin/assets/84225/ea868344-91f5-4e4b-b82c-5628f05a88bc"> on Github if you remove the `$` in front.
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1560053023)
Looks like `>>` is missing after `"maxconnections=2"` in
```
$ cd $DATA_DIR && echo "signet=1" >> bitcoin.conf && echo "maxconnections=2" bitcoin.conf && cd ~
```
Also, it’s easier to copy the commands using the (hover) copy icon <img width="46" alt="image" src="https://github.com/bitcoin/bitcoin/assets/84225/ea868344-91f5-4e4b-b82c-5628f05a88bc"> on Github if you remove the `$` in front.
💬 theuni commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1202937682)
Would it be possible to keep pointers/references to internal data out of this interface from the beginning? I'm not sure if that's a goal at this layer.
For blocktip for example, taking a quick look around, for the ui it looks like this eventually gets converted to:
`void(SynchronizationState, interfaces::BlockTip tip, double verification_progress)`
I wonder if we can do the same here?
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1202937682)
Would it be possible to keep pointers/references to internal data out of this interface from the beginning? I'm not sure if that's a goal at this layer.
For blocktip for example, taking a quick look around, for the ui it looks like this eventually gets converted to:
`void(SynchronizationState, interfaces::BlockTip tip, double verification_progress)`
I wonder if we can do the same here?
🤔 theuni reviewed a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1440486632)
From IRC:
> <TheCharlatan> Hey, how about moving the last two commits of #27636 (the only ones to touch the shutdown logic) to #27711? Then we can get #27636 merged and move ahead with #27576.
This is a good idea imo. I really don't want to slow this down, but I would benefit from a few extra days of catching up and testing the shutdown logic before reviewing/critiquing it in detail.
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1440486632)
From IRC:
> <TheCharlatan> Hey, how about moving the last two commits of #27636 (the only ones to touch the shutdown logic) to #27711? Then we can get #27636 merged and move ahead with #27576.
This is a good idea imo. I really don't want to slow this down, but I would benefit from a few extra days of catching up and testing the shutdown logic before reviewing/critiquing it in detail.
📝 fjahr opened a pull request: "init: Improve file descriptor limit handling"
(https://github.com/bitcoin/bitcoin/pull/27730)
I was playing with file descriptor limits to test another issue saw the following warning printed when I had configured a file descriptor limit of 150:
```
Warning: Reducing -maxconnections from 125 to -9, because of system limitations.
```
The warning could have been resolved with a one-line change but I found the surrounding code hard to reason about at first, so I added a few comments and reorganized the code a bit for better clarity.
Aside from the warning there is one further beh
...
(https://github.com/bitcoin/bitcoin/pull/27730)
I was playing with file descriptor limits to test another issue saw the following warning printed when I had configured a file descriptor limit of 150:
```
Warning: Reducing -maxconnections from 125 to -9, because of system limitations.
```
The warning could have been resolved with a one-line change but I found the surrounding code hard to reason about at first, so I added a few comments and reorganized the code a bit for better clarity.
Aside from the warning there is one further beh
...