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

💬 sipa 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-1557362646)
> 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?

I don't like that, conceptually. The balance should be correct, and if there are (reproducible) bugs related to that, they should be fixed. Exposing "is the software behaving correctly" RPCs shouldn't be needed if we'
...

💬 ryanofsky commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1557364808)
> @pinheadmz W:\BitcoinCore\_strDataDir\bitcoin.conf:
>
> ```
> blocksdir="W:/BitcoinCore/BlocksDir_/_.no_params"
> datadir="W:/BitcoinCore/DataDir_/_.no_params"
> ```
>
> 2023-05-08T22:11:59Z Using data directory W:\BitcoinCore\_strDataDir (this was read from hive) 2023-05-08T22:11:59Z Config file: W:\BitcoinCore\_strDataDir\bitcoin.conf 2023-05-08T22:11:59Z Config file arg: blocksdir="W:/BitcoinCore/BlocksDir_/_.no_params" 2023-05-08T22:11:59Z Config file arg: datadir="W:/BitcoinCore/
...

💬 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-1557383819)
The check wouldn't be used to check that the software is behaving properly, but that the user is behaving properly. For example, when calling `importdescriptors`, the burden is on the user to supply the correct rescan parameters and the check RPC could help to check that, no?

💬 theuni commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1557386699)
> > 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

I know I sound like a broken record here but... is that a guarantee? If so I think it should be documented. And again it feels brittle without some type of enforcement that the callback is
...

💬 sipa 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-1557388429)
@MarcoFalke Oh, I see! That's a different matter.

💬 brunoerg commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1200653417)
In 058fecdc7d2c99af9ae6aedc68c3600321e7c565: just a suggestion but I'd change `FindNextBlocksToDownload` to return `std::vector<const CBlockIndex*>` instead of passing it in the parameters.

💬 ryanofsky commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1557425054)
re: https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1539136159

> 2023-05-08T22:11:59Z Config file arg: datadir="W:/BitcoinCore/DataDir_/_.no_params" (**ignored**)

Thanks again @de-served for the new report. The problem you are seeing here is the problem I previously described:

> Unfortunately the behavior for `bitcoin-qt` is stupider than `bitcoind`, because if the user chooses a GUI datadir setting that is different _in any way_ from the default datadir setting, the `Intro
...

💬 hebasto commented on pull request "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es":
(https://github.com/bitcoin/bitcoin/pull/25390#issuecomment-1557425242)
Concept ~0. While the first commit introduces new code in `sync.h` that needs maintaining and unit test coverage, the following commits do not demonstrate convincing reasons for it.

> but it is possible to abuse the mutex and start using it to protect some more, possibly unrelated stuff (we already have this in the current code).

Concept ACK on improving this.