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

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

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

💬 furszy commented on pull request "index: improve initialization and pruning violation check":
(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)

💬 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

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

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

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

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

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

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

✅ hebasto closed a pull request: "ci: Check for `git` before its usage"
(https://github.com/bitcoin/bitcoin/pull/27718)

📝 MarnixCroes opened a pull request: "doc: Tor: fix link & generalize onion getnodeaddresses RPC"
(https://github.com/bitcoin/bitcoin/pull/27719)
- fix broken link about how to properly configure tor
- use `getnodeaddress 0 onion` instead of fetching a specific number, which makes more sense and for consistency with I2P and CJDNS doc

💬 brunoerg commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1200585547)
In c14ae132c5a5204a9a755c84c6de05fb30459221, is there a reason for not using `IsSnapshotActive()` here?

💬 sr-gi commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1557312833)
Adds invs to `m_recently_announced_invs` conditionally to them not being unconditionally relayable and rebases master.

💬 MarcoFalke commented on issue "wallet: Imports with pre-existing balance somtimes don't have any balance after a rescan":
(https://github.com/bitcoin/bitcoin/issues/19808#issuecomment-1557345831)
Closing due to lack of interest, progress and direction.

Pull requests with improvements are always welcome. Moreover, it is possible to re-open this issue or create a new issue referencing it, if there is fresh interest.

✅ MarcoFalke closed an issue: "wallet: Imports with pre-existing balance somtimes don't have any balance after a rescan"
(https://github.com/bitcoin/bitcoin/issues/19808)

💬 MarcoFalke commented on issue "wallet: Imports with pre-existing balance somtimes don't have any balance after a rescan":
(https://github.com/bitcoin/bitcoin/issues/19808#issuecomment-1557353185)
On a related note, what about adding a `checkbalance` RPC (or a `check` argument to the `getbalances` RPC) to check that the cached balance is correct? It could iterate over the utxo set for the check and then instruct for a `rescan` to restore missing transaction history and balance?

💬 willcl-ark commented on pull request "build: debug enable addrman consistency checks":
(https://github.com/bitcoin/bitcoin/pull/27300#issuecomment-1557357578)
OK pushed an update based on feedback here, in #24709 and #27586.

* PR description updated with new details of this pull.
* My opinion is that running these checks at a low enough frequency on mainnet debug builds is desirable and non-intrusive
* Selecting values of `1000` for both consistency checks worked with no noticable overhead for me, and appeared to run once every minute or so, although my test node is not listening to others may see higher frequency in "production debug" builds whi
...