💬 dergoegge commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202592163)
> This doesn't result in a behavior change, does it? As far as I can tell nodestate->m_is_inbound is initialized to the value of pfrom.IsInboundConn() when the CNodeState() is constructed -- is that understanding correct?
Yes that is correct, it does not change behavior (my original suggestion would have changed it slightly, as it would exclude manual conns but that wasn't intentional).
> I'm good with avoiding the use of CNodeState where possible though, if that is the reason for this cha
...
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202592163)
> This doesn't result in a behavior change, does it? As far as I can tell nodestate->m_is_inbound is initialized to the value of pfrom.IsInboundConn() when the CNodeState() is constructed -- is that understanding correct?
Yes that is correct, it does not change behavior (my original suggestion would have changed it slightly, as it would exclude manual conns but that wasn't intentional).
> I'm good with avoiding the use of CNodeState where possible though, if that is the reason for this cha
...
💬 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