💬 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/
...
(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?
(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
...
(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.
(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.
(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
...
(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.
(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.
🚀 achow101 merged a pull request: "rpc: add `descriptorprocesspsbt` rpc"
(https://github.com/bitcoin/bitcoin/pull/25796)
(https://github.com/bitcoin/bitcoin/pull/25796)
💬 hebasto commented on pull request "guix: remove redundant glibc patches":
(https://github.com/bitcoin/bitcoin/pull/27670#issuecomment-1557452526)
Post-merge ACK 3cfe366ec35eebfc0dad89ac7a09c32c45c30ea5. Having identical hashes for the merge commit ad7819d2f8abc311996cb6836315413641c2bebf for both `x86_64` and `arm64` build platforms:
```
05f7c42f98bf9fde66d8a6a895d10759f4f47d143a5c2acf87517942586bb1fc guix-build-ad7819d2f8ab/output/aarch64-linux-gnu/SHA256SUMS.part
e17f6abb7096779d312b703fa287d07e6c25fdae63bb2861dd7bcaad7f9588e0 guix-build-ad7819d2f8ab/output/aarch64-linux-gnu/bitcoin-ad7819d2f8ab-aarch64-linux-gnu-debug.tar.gz
98fa
...
(https://github.com/bitcoin/bitcoin/pull/27670#issuecomment-1557452526)
Post-merge ACK 3cfe366ec35eebfc0dad89ac7a09c32c45c30ea5. Having identical hashes for the merge commit ad7819d2f8abc311996cb6836315413641c2bebf for both `x86_64` and `arm64` build platforms:
```
05f7c42f98bf9fde66d8a6a895d10759f4f47d143a5c2acf87517942586bb1fc guix-build-ad7819d2f8ab/output/aarch64-linux-gnu/SHA256SUMS.part
e17f6abb7096779d312b703fa287d07e6c25fdae63bb2861dd7bcaad7f9588e0 guix-build-ad7819d2f8ab/output/aarch64-linux-gnu/bitcoin-ad7819d2f8ab-aarch64-linux-gnu-debug.tar.gz
98fa
...
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1557469158)
> Tripling the outgoing bandwidth load for peers selected for hb compact block relay seems concerning; not sure if I'd call it a blocker though.
To be clear, this is only a tripling in the case where where all your peers are roughly equally as fast, and triple of *what* is the most important thing.
The worst case scenario is going to be block after block of stuff we don't have in our mempool, and them being slow to validate, allowing multiple compact blocks to arrive before receiving a res
...
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1557469158)
> Tripling the outgoing bandwidth load for peers selected for hb compact block relay seems concerning; not sure if I'd call it a blocker though.
To be clear, this is only a tripling in the case where where all your peers are roughly equally as fast, and triple of *what* is the most important thing.
The worst case scenario is going to be block after block of stuff we don't have in our mempool, and them being slow to validate, allowing multiple compact blocks to arrive before receiving a res
...
💬 hebasto commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-1557479451)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-1557479451)
Concept ACK.
💬 hebasto commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#discussion_r1200717358)
Update the PR description:
> we check for the existence of `C:\Users\Username\AppData\Roaming\Bitcoin\blocks`
?
(https://github.com/bitcoin/bitcoin/pull/27064#discussion_r1200717358)
Update the PR description:
> we check for the existence of `C:\Users\Username\AppData\Roaming\Bitcoin\blocks`
?
💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1557487333)
> > > * Receive a fatal error callback before the parent function has completed
> >
> > 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.
I think possibly I'm not understanding the question. It seems self-evident that if some kernel function calls the `kernel::Notifications::fatalError` method, that function won't finish executing until the `kernel::Notifications::fatalError` method implem
...
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1557487333)
> > > * Receive a fatal error callback before the parent function has completed
> >
> > 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.
I think possibly I'm not understanding the question. It seems self-evident that if some kernel function calls the `kernel::Notifications::fatalError` method, that function won't finish executing until the `kernel::Notifications::fatalError` method implem
...
📝 furszy opened a pull request: "index: prevent race by calling 'CustomInit' prior setting 'synced' flag"
(https://github.com/bitcoin/bitcoin/pull/27720)
Decoupled from #27607.
Fixed a potential race condition in master (not possible so far) that could become a actual issue soon.
Where the index's `CustomAppend` method could be called (from `BlockConnected`) before its
`CustomInit` method, causing the index to try to update itself before it is initialized.
This could happen because we set the index `m_synced` flag (which enables `BlockConnected` events)
before calling to the child class init function (`CustomInit`). So, for example, the
...
(https://github.com/bitcoin/bitcoin/pull/27720)
Decoupled from #27607.
Fixed a potential race condition in master (not possible so far) that could become a actual issue soon.
Where the index's `CustomAppend` method could be called (from `BlockConnected`) before its
`CustomInit` method, causing the index to try to update itself before it is initialized.
This could happen because we set the index `m_synced` flag (which enables `BlockConnected` events)
before calling to the child class init function (`CustomInit`). So, for example, the
...
💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1557536125)
> > Tripling the outgoing bandwidth load for peers selected for hb compact block relay seems concerning; not sure if I'd call it a blocker though.
>
> To be clear, this is only a tripling in the case where where all your peers are roughly equally as fast, and triple of _what_ is the most important thing.
In an "everyone's honest" environment, I think the most likely reason for there to be enough of a delay between `cmpctblock / getblocktxns / blocktxns` that there'll be another `cmpctblock
...
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1557536125)
> > Tripling the outgoing bandwidth load for peers selected for hb compact block relay seems concerning; not sure if I'd call it a blocker though.
>
> To be clear, this is only a tripling in the case where where all your peers are roughly equally as fast, and triple of _what_ is the most important thing.
In an "everyone's honest" environment, I think the most likely reason for there to be enough of a delay between `cmpctblock / getblocktxns / blocktxns` that there'll be another `cmpctblock
...
💬 furszy commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1557553941)
Feedback tackled, thanks ryanofsky and mzumzande for your deep review!
This PR now depends on #27720 and #27708.
List of changes since the last review:
* Created #27708 so the post-init shutdown doesn’t introduce any tangible difference for user and tests. (Per https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1198184049 discussion).
* Moved the `LoadMempool()` call from the blockstorage import function to init.cpp (per https://github.com/bitcoin/bitcoin/pull/27607#discussion
...
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1557553941)
Feedback tackled, thanks ryanofsky and mzumzande for your deep review!
This PR now depends on #27720 and #27708.
List of changes since the last review:
* Created #27708 so the post-init shutdown doesn’t introduce any tangible difference for user and tests. (Per https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1198184049 discussion).
* Moved the `LoadMempool()` call from the blockstorage import function to init.cpp (per https://github.com/bitcoin/bitcoin/pull/27607#discussion
...
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200766163)
For serializing the strings for the prefix, the stream type and client version don't matter so I just didn't use them.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200766163)
For serializing the strings for the prefix, the stream type and client version don't matter so I just didn't use them.
💬 theuni commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1557558059)
> > I know I sound like a broken record here but... is that a guarantee? If so I think it should be documented.
>
> I think possibly I'm not understanding the question. It seems self-evident that if some kernel function calls the `kernel::Notifications::fatalError` method, that function won't finish executing until the `kernel::Notifications::fatalError` method implementation completes and returns. Function calls in c++ are blocking, so functions don't return until the functions they call ret
...
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1557558059)
> > I know I sound like a broken record here but... is that a guarantee? If so I think it should be documented.
>
> I think possibly I'm not understanding the question. It seems self-evident that if some kernel function calls the `kernel::Notifications::fatalError` method, that function won't finish executing until the `kernel::Notifications::fatalError` method implementation completes and returns. Function calls in c++ are blocking, so functions don't return until the functions they call ret
...
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200777487)
I think it's fine to leave it as is.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200777487)
I think it's fine to leave it as is.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200777808)
Adopted these suggestions.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200777808)
Adopted these suggestions.