🤔 theuni reviewed a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1436636736)
A few high-level conceptual questions that weren't obvious from a quick skim:
What's the threading model of the notification interface? For example, might it be possible that I:
- Receive a fatal error callback before the parent function has completed
- Not yet received the fatal callback before calling another function
I guess your answer to the latter would be "the caller should've received an error as well". See my other comment about my concern there.
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1436636736)
A few high-level conceptual questions that weren't obvious from a quick skim:
What's the threading model of the notification interface? For example, might it be possible that I:
- Receive a fatal error callback before the parent function has completed
- Not yet received the fatal callback before calling another function
I guess your answer to the latter would be "the caller should've received an error as well". See my other comment about my concern there.
💬 theuni commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200518879)
Sorry for the late conceptual review, but I'm worried about how true this is and how well it's enforced? IIRC last time I played with my clang-tidy plugin, there were at least a few places where StartShutdown() was called (maybe in a nested function) but then the function didn't return immediately.
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200518879)
Sorry for the late conceptual review, but I'm worried about how true this is and how well it's enforced? IIRC last time I played with my clang-tidy plugin, there were at least a few places where StartShutdown() was called (maybe in a nested function) but then the function didn't return immediately.
💬 MarcoFalke commented on pull request "ci: Ensure git is installed before usage":
(https://github.com/bitcoin/bitcoin/pull/27718#issuecomment-1557239940)
The warning is harmless. If `git` isn't installed, we can be sure that `base-install-done` is not `true`
(https://github.com/bitcoin/bitcoin/pull/27718#issuecomment-1557239940)
The warning is harmless. If `git` isn't installed, we can be sure that `base-install-done` is not `true`
💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200535398)
This seems like something that should be improved. Though, as it would be a behavior change, it may be better to do it separate from this refactoring change?
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200535398)
This seems like something that should be improved. Though, as it would be a behavior change, it may be better to do it separate from this refactoring change?
👍 instagibbs approved a pull request: "test: p2p: check misbehavior for non-continuous headers messages"
(https://github.com/bitcoin/bitcoin/pull/27712#pullrequestreview-1436656563)
ACK a97c59f12d50d11d8859f4bbfb9fcf66de667ca0
(https://github.com/bitcoin/bitcoin/pull/27712#pullrequestreview-1436656563)
ACK a97c59f12d50d11d8859f4bbfb9fcf66de667ca0
💬 instagibbs commented on pull request "test: p2p: check misbehavior for non-continuous headers messages":
(https://github.com/bitcoin/bitcoin/pull/27712#discussion_r1200531137)
absolutely don't have to take, I just like using this pythonism
```suggestion
block_headers = [from_hex(CBlockHeader(), self.nodes[0].getblockheader(block_hash, False) for block_hash in block_hashes]
```
(https://github.com/bitcoin/bitcoin/pull/27712#discussion_r1200531137)
absolutely don't have to take, I just like using this pythonism
```suggestion
block_headers = [from_hex(CBlockHeader(), self.nodes[0].getblockheader(block_hash, False) for block_hash in block_hashes]
```
💬 instagibbs commented on pull request "test: p2p: check misbehavior for non-continuous headers messages":
(https://github.com/bitcoin/bitcoin/pull/27712#discussion_r1200536118)
do we want to assert anything about not disconnecting, or the total ban score?
(https://github.com/bitcoin/bitcoin/pull/27712#discussion_r1200536118)
do we want to assert anything about not disconnecting, or the total ban score?
💬 hebasto commented on pull request "ci: Ensure git is installed before usage":
(https://github.com/bitcoin/bitcoin/pull/27718#issuecomment-1557248002)
> The warning is harmless. If `git` isn't installed, we can be sure that `base-install-done` is not `true`
You mean, checking for git binary, `command -v git`?
(https://github.com/bitcoin/bitcoin/pull/27718#issuecomment-1557248002)
> The warning is harmless. If `git` isn't installed, we can be sure that `base-install-done` is not `true`
You mean, checking for git binary, `command -v git`?
💬 theuni commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200543291)
I'm more worried about the enforcement question. I'm nearly positive that this assumption is not true now, and even if so there's nothing other than review keeping it true in the future.
I'll work on generating a more current shutdown-bubble-up demo to demonstrate the problem.
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200543291)
I'm more worried about the enforcement question. I'm nearly positive that this assumption is not true now, and even if so there's nothing other than review keeping it true in the future.
I'll work on generating a more current shutdown-bubble-up demo to demonstrate the problem.
💬 hebasto commented on pull request "ci: Ensure git is installed before usage":
(https://github.com/bitcoin/bitcoin/pull/27718#issuecomment-1557259482)
> The warning is harmless. If `git` isn't installed, we can be sure that `base-install-done` is not `true`
Reworked.
(https://github.com/bitcoin/bitcoin/pull/27718#issuecomment-1557259482)
> The warning is harmless. If `git` isn't installed, we can be sure that `base-install-done` is not `true`
Reworked.
💬 furszy commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1200555420)
> Is my understanding correct that this race cannot actually happen in practice on master right now, because the indexes are set up (Init()) by the init thread, which starts the networking and loadblk threads only later - so I can't see from which other thread BlockConnected signals could come from at this stage.
Yeah, the race cannot realistically happen in master. The only process that occurs before the indexes initialization is the chain state loading process, which doesn't signal any blo
...
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1200555420)
> Is my understanding correct that this race cannot actually happen in practice on master right now, because the indexes are set up (Init()) by the init thread, which starts the networking and loadblk threads only later - so I can't see from which other thread BlockConnected signals could come from at this stage.
Yeah, the race cannot realistically happen in master. The only process that occurs before the indexes initialization is the chain state loading process, which doesn't signal any blo
...
💬 furszy commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1200556364)
good catch 👌🏼 .
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1200556364)
good catch 👌🏼 .
💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200564788)
Yes, it is certainly possible that this is not true now. However, then again, if it is a problem on current `master` and this pull request isn't changing that, nor making it worse, then it seems better to do in a separate pull/issue? (I agree it should be addressed, btw)
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200564788)
Yes, it is certainly possible that this is not true now. However, then again, if it is a problem on current `master` and this pull request isn't changing that, nor making it worse, then it seems better to do in a separate pull/issue? (I agree it should be addressed, btw)
💬 MarcoFalke commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1557285593)
Red CI can be ignored. This is an upstream bug, see https://github.com/bitcoin/bitcoin/issues/27492#issuecomment-1527772394
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1557285593)
Red CI can be ignored. This is an upstream bug, see https://github.com/bitcoin/bitcoin/issues/27492#issuecomment-1527772394
💬 furszy commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1200568842)
> I don't think this is possible to trigger with the way the function is used, but if upper_block already has NO_DATA, it would return that block, maybe nullptr would be more natural? (also applies to GetFirstStoredBlock).
yeah, that is something that already exists. Good eye.
It shouldn't be possible to trigger as `upper_block` is always equal to the chain tip and, as far as I know, we never prune the tip. But.. a bug is a bug, better to fix it now.
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1200568842)
> I don't think this is possible to trigger with the way the function is used, but if upper_block already has NO_DATA, it would return that block, maybe nullptr would be more natural? (also applies to GetFirstStoredBlock).
yeah, that is something that already exists. Good eye.
It shouldn't be possible to trigger as `upper_block` is always equal to the chain tip and, as far as I know, we never prune the tip. But.. a bug is a bug, better to fix it now.
🤔 ryanofsky reviewed a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1436684577)
re: https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1436636736
> A few high-level conceptual questions that weren't obvious from a quick skim:
>
> What's the threading model of the notification interface? For example, might it be possible that I:
>
>* Receive a fatal error callback before the parent function has completed
>
>* Not yet received the fatal callback before calling another function
The first thing should always happen and the second thing should never h
...
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1436684577)
re: https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1436636736
> A few high-level conceptual questions that weren't obvious from a quick skim:
>
> What's the threading model of the notification interface? For example, might it be possible that I:
>
>* Receive a fatal error callback before the parent function has completed
>
>* Not yet received the fatal callback before calling another function
The first thing should always happen and the second thing should never h
...
💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200547867)
re: https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200518879
> I'm more worried about the enforcement question. I'm nearly positive that this assumption is not true now, and even if so there's nothing other than review keeping it true in the future.
Hmm, I wasn't aware of these cases, but if we want to add an enforcement mechanism that could be interesting. I guess one way to do it would be to make the method private and add `FATAL_THROW` `FATAL_RETURN` macros that are friends
...
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200547867)
re: https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200518879
> I'm more worried about the enforcement question. I'm nearly positive that this assumption is not true now, and even if so there's nothing other than review keeping it true in the future.
Hmm, I wasn't aware of these cases, but if we want to add an enforcement mechanism that could be interesting. I guess one way to do it would be to make the method private and add `FATAL_THROW` `FATAL_RETURN` macros that are friends
...
🤔 hebasto reviewed a pull request: "init: Error if ignored bitcoin.conf file is found"
(https://github.com/bitcoin/bitcoin/pull/27302#pullrequestreview-1436728054)
Approach ACK 0319de5cbedd1a8f8766cfec61151c58b3fb27ef.
(https://github.com/bitcoin/bitcoin/pull/27302#pullrequestreview-1436728054)
Approach ACK 0319de5cbedd1a8f8766cfec61151c58b3fb27ef.
💬 hebasto commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1200573624)
Any possibility to make the error message translatable, as for others `ConfigError` calls?
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1200573624)
Any possibility to make the error message translatable, as for others `ConfigError` calls?
💬 hebasto commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1200571325)
This line seems redundant when `allowignoredconf=1` has been provided actually:
```
$ ./src/bitcoind -regtest -allowignoredconf=1
2023-05-22T14:06:27Z Warning: Data directory "/home/hebasto/TEST" contains a "bitcoin.conf" file which is ignored, because a different configuration file "/home/hebasto/.bitcoin/bitcoin.conf" from data directory "/home/hebasto/.bitcoin" is being used instead. Possible ways to address this would be to:
- Delete or rename the "bitcoin.conf" file in data directory "/
...
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1200571325)
This line seems redundant when `allowignoredconf=1` has been provided actually:
```
$ ./src/bitcoind -regtest -allowignoredconf=1
2023-05-22T14:06:27Z Warning: Data directory "/home/hebasto/TEST" contains a "bitcoin.conf" file which is ignored, because a different configuration file "/home/hebasto/.bitcoin/bitcoin.conf" from data directory "/home/hebasto/.bitcoin" is being used instead. Possible ways to address this would be to:
- Delete or rename the "bitcoin.conf" file in data directory "/
...