💬 mzumsande commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202606662)
I think that this can currently be broken by `FetchBlock()` / `getblockfrompeer`, which would now allow multiple block requests in parallel, breaking the `MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK` limit. We could probably make sure in `FetchBlock()` that MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK isn't breached (or maybe disallow all parallel request).
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202606662)
I think that this can currently be broken by `FetchBlock()` / `getblockfrompeer`, which would now allow multiple block requests in parallel, breaking the `MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK` limit. We could probably make sure in `FetchBlock()` that MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK isn't breached (or maybe disallow all parallel request).
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1202616997)
Good point.
I've changed the prefix to be made with a `DataStream` and for `GetPrefixCursor` to take `Span<const std::byte>` rather than a stream. Also updated to use `DataStream key` everywhere, I think that got lost in a rebase.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1202616997)
Good point.
I've changed the prefix to be made with a `DataStream` and for `GetPrefixCursor` to take `Span<const std::byte>` rather than a stream. Also updated to use `DataStream key` everywhere, I think that got lost in a rebase.
💬 Crypt-iQ commented on issue "bitcoind hangs waiting for `g_requests.empty()`":
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1559731418)
I was able to fix this with my branch here https://github.com/Crypt-iQ/bitcoin/commit/4fd7adb30f584664421b6431bce8ebcbf7ceef2f by adding a `evhttp_connection_set_closecb` callback. I didn't need to modify the logic related to `EV_READ` and no functional test errors are introduced on my machine
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1559731418)
I was able to fix this with my branch here https://github.com/Crypt-iQ/bitcoin/commit/4fd7adb30f584664421b6431bce8ebcbf7ceef2f by adding a `evhttp_connection_set_closecb` callback. I didn't need to modify the logic related to `EV_READ` and no functional test errors are introduced on my machine
🤔 theuni reviewed a pull request: "Parallel compact block downloads, take 3"
(https://github.com/bitcoin/bitcoin/pull/27626#pullrequestreview-1440012745)
Very shallow/mechanical code review ACK. Left a few nits for some head-scratchers.
I was mostly looking for misuses of the multimap. Zero opinion about the logical change itself.
(https://github.com/bitcoin/bitcoin/pull/27626#pullrequestreview-1440012745)
Very shallow/mechanical code review ACK. Left a few nits for some head-scratchers.
I was mostly looking for misuses of the multimap. Zero opinion about the logical change itself.
💬 theuni commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202580653)
Too much. This one is just too much :p
(Call it a)Nit: Could you please break this up with a temp const ref or so for the sake of readability?
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202580653)
Too much. This one is just too much :p
(Call it a)Nit: Could you please break this up with a temp const ref or so for the sake of readability?
💬 theuni commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202565711)
Nit: it took me a minute to parse this. It'd be easier to read as a static function with a name imo.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202565711)
Nit: it took me a minute to parse this. It'd be easier to read as a static function with a name imo.
💬 jamesob commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1559752787)
Rebased
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1559752787)
Rebased
💬 achow101 commented on pull request "rpc: Fix invalid bech32 address handling":
(https://github.com/bitcoin/bitcoin/pull/27727#issuecomment-1559757854)
ACK eeee55f9288740747b6e8d806ce8177fd92635cf
(https://github.com/bitcoin/bitcoin/pull/27727#issuecomment-1559757854)
ACK eeee55f9288740747b6e8d806ce8177fd92635cf
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202685072)
ah, good catch! I think the only sensible change here is to restrict parallel fetches, but that seems like a more serious API change? @Sjors ?
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202685072)
ah, good catch! I think the only sensible change here is to restrict parallel fetches, but that seems like a more serious API change? @Sjors ?
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202698973)
restricted it to a single invocation per block at a time. Alternative seems more difficult to think about.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202698973)
restricted it to a single invocation per block at a time. Alternative seems more difficult to think about.
💬 jamesob commented on pull request "validation: have LoadBlockIndex account for snapshot use":
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202700402)
@sdaftuar:
> If we have a snapshot in place (and so some block index is marked IsAssumedValid), but we have downloaded blocks that haven't yet been activated (eg on the background-validation chainstate), this logic prevents us from adding those downloaded-but-not-yet verified blocks to setBlockIndexCandidate for the background chainstate
I'm having a hard time following how this could happen. If the bg chainstate has some downloaded-but-not-activated blocks, they are definitionally beneath
...
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1202700402)
@sdaftuar:
> If we have a snapshot in place (and so some block index is marked IsAssumedValid), but we have downloaded blocks that haven't yet been activated (eg on the background-validation chainstate), this logic prevents us from adding those downloaded-but-not-yet verified blocks to setBlockIndexCandidate for the background chainstate
I'm having a hard time following how this could happen. If the bg chainstate has some downloaded-but-not-activated blocks, they are definitionally beneath
...
💬 jamesob commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1559813326)
Cross-linking to some discussion on how we load the block index that @sdaftuar has surfaced: https://github.com/bitcoin/bitcoin/pull/23174/files#r1198450499
If anyone has encountered an issue like this while testing this branch, a comment would be welcome.
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1559813326)
Cross-linking to some discussion on how we load the block index that @sdaftuar has surfaced: https://github.com/bitcoin/bitcoin/pull/23174/files#r1198450499
If anyone has encountered an issue like this while testing this branch, a comment would be welcome.
🤔 fanquake reviewed a pull request: "doc: Add Python install notes to build-osx.md."
(https://github.com/bitcoin/bitcoin/pull/27728#pullrequestreview-1440185420)
> 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 single platf
...
(https://github.com/bitcoin/bitcoin/pull/27728#pullrequestreview-1440185420)
> 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 single platf
...
💬 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.