🤔 ryanofsky reviewed a pull request: "init: Error if ignored bitcoin.conf file is found"
(https://github.com/bitcoin/bitcoin/pull/27302#pullrequestreview-1371794249)
Updated 0319de5cbedd1a8f8766cfec61151c58b3fb27ef -> eefe56967b4eb4b5144325cde4f40fc1cbde3e65 ([`pr/ignoredconf.13`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.13) -> [`pr/ignoredconf.14`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.14), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ignoredconf.13..pr/ignoredconf.14)) with suggestions

Thanks for the reviews!

💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1157865333)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1150550636

> So perhaps could go next to that in a future PR?

Sure, moved there now

💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1157769058)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1150313450

> If you re-touch, perhaps this could read:
>
> 'Test error is triggered when the datadir in use contains a bitcoin.conf file that would be ignored '

Thanks applied suggestion

💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1202451370)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194040447

Thanks! Applied change

💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1157784564)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1150313881

> On a re-touch similar re-wording could be done here?

I don't think I would add "used" here or imply anything about the datadir here since the message is supposed to be about the config file not the datadir. The gist of the message is supposed to be: "you have two configuration files, and one is being ignored, so please merge them or get rid of the one you don't want."

Extra notes about the datadir I think are he
...

💬 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
...

💬 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
...

💬 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

💬 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
...

💬 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.

💬 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
...

💬 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).

💬 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.

💬 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

🤔 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.

💬 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?

💬 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.

💬 jamesob commented on pull request "assumeutxo (2)":
(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

💬 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 ?