💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1202451739)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194151784
> I think this will unnecessarily repeat the entire path, could maybe drop `from %4$s`, I got this message just now:
Feel free to suggest the code change that you would like to see, but I think saying configuration file {initial_datadir}/bitcoin.conf from data directory {initial_datadir}" is more helpful than saying "configuration file {initial_datdir}/bitcoin.conf" because the latter tells you which configuration fil
...
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1202451739)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194151784
> I think this will unnecessarily repeat the entire path, could maybe drop `from %4$s`, I got this message just now:
Feel free to suggest the code change that you would like to see, but I think saying configuration file {initial_datadir}/bitcoin.conf from data directory {initial_datadir}" is more helpful than saying "configuration file {initial_datdir}/bitcoin.conf" because the latter tells you which configuration fil
...
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1202451976)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1200571325
> This line seems redundant when `allowignoredconf=1` has been provided actually:
Feel free to suggest a code change that you would like to see here.
IMO, the -allowignoredconf option is meant to provide an escape hatch for backwards compatibility when your bitcoin configuration is complicated and confusing and you don't have time to clean it up. I think implementation of the option should be as simple as possible
...
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1202451976)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1200571325
> This line seems redundant when `allowignoredconf=1` has been provided actually:
Feel free to suggest a code change that you would like to see here.
IMO, the -allowignoredconf option is meant to provide an escape hatch for backwards compatibility when your bitcoin configuration is complicated and confusing and you don't have time to clean it up. I think implementation of the option should be as simple as possible
...
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1202453349)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194133266
> nit: indent should be +2 more spaces
Thanks! Fixed this
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1202453349)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194133266
> nit: indent should be +2 more spaces
Thanks! Fixed this
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1202452305)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1200573624
> Any possibility to make the error message translatable, as for others `ConfigError` calls?
Are you suggesting actually translating this message, or just making a code change to make it easier to translations the future?
Obviously any message is translatable, but I didn't want this particular message translated because it I think it should only show up in niche cases where users have manually created multiple data
...
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1202452305)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1200573624
> Any possibility to make the error message translatable, as for others `ConfigError` calls?
Are you suggesting actually translating this message, or just making a code change to make it easier to translations the future?
Obviously any message is translatable, but I didn't want this particular message translated because it I think it should only show up in niche cases where users have manually created multiple data
...
💬 sdaftuar commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202566303)
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?
I'm good with avoiding the use of CNodeState where possible though, if that is the reason for this change.
If these are different somehow, then perhaps we'd want to change the logic in `IsBlockRequestedFromOutbound` to match it.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1202566303)
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?
I'm good with avoiding the use of CNodeState where possible though, if that is the reason for this change.
If these are different somehow, then perhaps we'd want to change the logic in `IsBlockRequestedFromOutbound` to match it.
💬 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?