💬 fanquake commented on pull request "doc: Add Python install notes to build-osx.md.":
(https://github.com/bitcoin/bitcoin/pull/27728#discussion_r1202700919)
`Bitcoin/gui/` - This isn't a directory in our source code?
(https://github.com/bitcoin/bitcoin/pull/27728#discussion_r1202700919)
`Bitcoin/gui/` - This isn't a directory in our source code?
💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202712706)
It's fine by me, though currently we have no way of telling whether a request is in progress. Maybe bring this up in #27652 too.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202712706)
It's fine by me, though currently we have no way of telling whether a request is in progress. Maybe bring this up in #27652 too.
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202730525)
After talking offline with @Sjors , his use-case does involve asking multiple peers to detect stale tips.
Instead, any call of `getblockbypeer` will cause all of the same block requests to be forgotten/ignored. It's up to the user of the RPC to not have this interfere with syncing of their own node.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202730525)
After talking offline with @Sjors , his use-case does involve asking multiple peers to detect stale tips.
Instead, any call of `getblockbypeer` will cause all of the same block requests to be forgotten/ignored. It's up to the user of the RPC to not have this interfere with syncing of their own node.
💬 evansmj commented on pull request "doc: Add Python install notes to build-osx.md.":
(https://github.com/bitcoin/bitcoin/pull/27728#issuecomment-1559846291)
> > When installing Bitcoin Core on OSX, a new developer may run into a situation where the deploy dependencies pip3 install ds_store mac_alias may be installed to an incorrect version of Python on the machine.
>
> This sounds like a local issue with your own Python version/package management.
>
> Concept NACK on changing this to install pyenv, and become more involved in regards to package installation. Note that this also isn't a change we'd make in isolation to the build docs for a sing
...
(https://github.com/bitcoin/bitcoin/pull/27728#issuecomment-1559846291)
> > When installing Bitcoin Core on OSX, a new developer may run into a situation where the deploy dependencies pip3 install ds_store mac_alias may be installed to an incorrect version of Python on the machine.
>
> This sounds like a local issue with your own Python version/package management.
>
> Concept NACK on changing this to install pyenv, and become more involved in regards to package installation. Note that this also isn't a change we'd make in isolation to the build docs for a sing
...
💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202751434)
That works for me.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202751434)
That works for me.
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1559872914)
Ready for re-ACKs now that `getblockfrompeer` issue is fixed
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1559872914)
Ready for re-ACKs now that `getblockfrompeer` issue is fixed
💬 sdaftuar commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1559886304)
ACK d7f359b35e8b1e9acc4d397de262cd9ba9bbcb83
To reiterate, I think we should continue to keep an eye on bandwidth usage (and load on outbound peers) after this patch and revisit whether any additional tuning of this behavior is needed prior to release. However it'll be useful to have this in master so that we can monitor for the next few months.
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1559886304)
ACK d7f359b35e8b1e9acc4d397de262cd9ba9bbcb83
To reiterate, I think we should continue to keep an eye on bandwidth usage (and load on outbound peers) after this patch and revisit whether any additional tuning of this behavior is needed prior to release. However it'll be useful to have this in master so that we can monitor for the next few months.
💬 sdaftuar commented on pull request "validation: have LoadBlockIndex account for snapshot use":
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202781921)
> they are definitionally beneath the first_assumed_valid_height (since we don't attach blocks above that height to the bg chainstate)
I don't believe this is true; after we download a block and before we validate the block, the CBlockIndex entry for that block will have the AssumedValid bit set to true. This means that if you were to download a bunch of blocks and then shutdown prior to those blocks being validated on the bg chainstate, that when you restart you'll be in this state.
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202781921)
> they are definitionally beneath the first_assumed_valid_height (since we don't attach blocks above that height to the bg chainstate)
I don't believe this is true; after we download a block and before we validate the block, the CBlockIndex entry for that block will have the AssumedValid bit set to true. This means that if you were to download a bunch of blocks and then shutdown prior to those blocks being validated on the bg chainstate, that when you restart you'll be in this state.
🤔 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