💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2379765693)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1778636559
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2379765693)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1778636559
💬 itornaza commented on issue "depends: llvm-ranlib (etc): "No such file or directory" on Intel macOS 15.0":
(https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2379782629)
Following the [macOS Build Guide](https://github.com/bitcoin/bitcoin/blob/master/doc/build-osx.md#2-homebrew-package-manager) I am using the `llvm` installed from brew and in my `.zshrc` I have an export to override the default macos llvm `export PATH="$(brew --prefix)/opt/llvm/bin:$PATH"`.
Using the `which` command with the missing tools, I get the corresponding tool paths that are of course not the ones in the `usr/bin/` directory.
```
% which llvm-ranlib
/opt/homebrew/opt/llvm/bin/llv
...
(https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2379782629)
Following the [macOS Build Guide](https://github.com/bitcoin/bitcoin/blob/master/doc/build-osx.md#2-homebrew-package-manager) I am using the `llvm` installed from brew and in my `.zshrc` I have an export to override the default macos llvm `export PATH="$(brew --prefix)/opt/llvm/bin:$PATH"`.
Using the `which` command with the missing tools, I get the corresponding tool paths that are of course not the ones in the `usr/bin/` directory.
```
% which llvm-ranlib
/opt/homebrew/opt/llvm/bin/llv
...
💬 davidgumberg commented on pull request "Don't zero-after-free `DataStream`: ~25% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-2379857634)
The CI failure on ARM is related, and I am able to reproduce locally. It is from a `-Warray-bounds` warning that I believe is spurious, I'm trying to create a minimal reproduction.
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-2379857634)
The CI failure on ARM is related, and I am able to reproduce locally. It is from a `-Warray-bounds` warning that I believe is spurious, I'm trying to create a minimal reproduction.
💬 ryanofsky commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1779015530)
Thanks, good catch. I knew that pthreads required the lock to be held while notifying to avoid the lost wakeup bug (["if predictable scheduling behavior is required"](https://linux.die.net/man/3/pthread_cond_broadcast)), and that c++ dropped this requirement, even recommending against it saying ["Doing so may be a pessimization..."](https://en.cppreference.com/w/cpp/thread/condition_variable/notify_all).
But I didn't know that c++ still requires the lock to be held *before* notifying, even th
...
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1779015530)
Thanks, good catch. I knew that pthreads required the lock to be held while notifying to avoid the lost wakeup bug (["if predictable scheduling behavior is required"](https://linux.die.net/man/3/pthread_cond_broadcast)), and that c++ dropped this requirement, even recommending against it saying ["Doing so may be a pessimization..."](https://en.cppreference.com/w/cpp/thread/condition_variable/notify_all).
But I didn't know that c++ still requires the lock to be held *before* notifying, even th
...
💬 theuni commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1779026190)
Yeah, I think the "Doing so may be a pessimization..." language encourages this footgun, sadly.
See [here](https://github.com/isocpp/CppCoreGuidelines/issues/1272) for a discussion about inverting the language.
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1779026190)
Yeah, I think the "Doing so may be a pessimization..." language encourages this footgun, sadly.
See [here](https://github.com/isocpp/CppCoreGuidelines/issues/1272) for a discussion about inverting the language.
💬 ryanofsky commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1779040288)
Either approach seems ok. I probably wouldn't have added the null check, but it does a provide a small convenience if miner is running while the node is restarted, since waitTipChanged will patiently wait for startup to complete and a new block to be connected, instead of returning 0 on the first waitTipChanged call and requiring the miner to make a second call.
Whether we keep this check or drop it, it would probably be good to update the `waitTipChanged` documentation for the method to say
...
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1779040288)
Either approach seems ok. I probably wouldn't have added the null check, but it does a provide a small convenience if miner is running while the node is restarted, since waitTipChanged will patiently wait for startup to complete and a new block to be connected, instead of returning 0 on the first waitTipChanged call and requiring the miner to make a second call.
Whether we keep this check or drop it, it would probably be good to update the `waitTipChanged` documentation for the method to say
...
💬 ryanofsky commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1779056857)
re: https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778162781
> In terms of how the Template Provider works, it used to rely on this check, but now it just waits for `getTip()` to return a value before calling `waitTipChanged()`. .
>
> When implemented as part of Bitcoin Core, as in #29432, it can't call `waitTipChanged()` earlier, because it has no way of knowing what to set for `current_tip`.
I may be misunderstanding, but this doesn't seem right to me. If a caller just wan
...
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1779056857)
re: https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778162781
> In terms of how the Template Provider works, it used to rely on this check, but now it just waits for `getTip()` to return a value before calling `waitTipChanged()`. .
>
> When implemented as part of Bitcoin Core, as in #29432, it can't call `waitTipChanged()` earlier, because it has no way of knowing what to set for `current_tip`.
I may be misunderstanding, but this doesn't seem right to me. If a caller just wan
...
🤔 naiyoma reviewed a pull request: "RPC: improve SFFO arg parsing, error catching and coverage"
(https://github.com/bitcoin/bitcoin/pull/30844#pullrequestreview-2334508914)
TACK [https://github.com/bitcoin/bitcoin/pull/30844/commits/cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e](https://github.com/bitcoin/bitcoin/pull/30844/commits/cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e)
Nice clean-up, as well as adding more assertions to cover additional edge cases.
(https://github.com/bitcoin/bitcoin/pull/30844#pullrequestreview-2334508914)
TACK [https://github.com/bitcoin/bitcoin/pull/30844/commits/cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e](https://github.com/bitcoin/bitcoin/pull/30844/commits/cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e)
Nice clean-up, as well as adding more assertions to cover additional edge cases.
📝 Mackain opened a pull request: "doc: Minor update to doc/README.md"
(https://github.com/bitcoin/bitcoin/pull/30992)
A small change to the first paragraph of the Setup part of the README that has been bugging me for a while.
The disk space required for the Bitcoin transactions can no longer be described as "a few" hundred gigabytes.
So I thought it was time it was changed to "several" instead.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
h
...
(https://github.com/bitcoin/bitcoin/pull/30992)
A small change to the first paragraph of the Setup part of the README that has been bugging me for a while.
The disk space required for the Bitcoin transactions can no longer be described as "a few" hundred gigabytes.
So I thought it was time it was changed to "several" instead.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
h
...
💬 hodlinator commented on pull request "DO NOT MERGE: Windows bitcoind stall debugging":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2380085073)
https://github.com/hodlinator/bitcoin/actions/runs/11069726468/job/30757882453#step:15:114 :
`subprocess.TimeoutExpired: Command '['D:\\a\\bitcoin\\bitcoin\\build\\test\\cache\\node0\\Procdump.exe', '-mm', '7904', 'D:\\a\\bitcoin\\bitcoin\\build\\test\\cache\\node0\\bitcoind.dmp']' timed out after 4800 seconds`
The Procdump.exe process itself times out when trying to produce the dump. :facepalm:
Could be that it's popping up a GUI dialogue on first run, will investigate further.
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2380085073)
https://github.com/hodlinator/bitcoin/actions/runs/11069726468/job/30757882453#step:15:114 :
`subprocess.TimeoutExpired: Command '['D:\\a\\bitcoin\\bitcoin\\build\\test\\cache\\node0\\Procdump.exe', '-mm', '7904', 'D:\\a\\bitcoin\\bitcoin\\build\\test\\cache\\node0\\bitcoind.dmp']' timed out after 4800 seconds`
The Procdump.exe process itself times out when trying to produce the dump. :facepalm:
Could be that it's popping up a GUI dialogue on first run, will investigate further.
💬 jonatack commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2380088608)
> You could also run tor-only in this case, but that has its own disadvantages and `-onlynet=onion` shares the exact same downside that if everyone did it, the network would split.
Our docs address this to an extent [here](https://github.com/bitcoin/bitcoin/blob/master/doc/i2p.md#additional-configuration-options-related-to-i2p) and [here](https://github.com/bitcoin/bitcoin/blob/master/doc/cjdns.md#additional-configuration-options-related-to-cjdns). As those docs point out, `-onlynet` only af
...
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2380088608)
> You could also run tor-only in this case, but that has its own disadvantages and `-onlynet=onion` shares the exact same downside that if everyone did it, the network would split.
Our docs address this to an extent [here](https://github.com/bitcoin/bitcoin/blob/master/doc/i2p.md#additional-configuration-options-related-to-i2p) and [here](https://github.com/bitcoin/bitcoin/blob/master/doc/cjdns.md#additional-configuration-options-related-to-cjdns). As those docs point out, `-onlynet` only af
...
💬 jonatack commented on issue "V2 Only Option":
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2380125677)
I've commented on this feature request at https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2380088608.
Given the insistence and somewhat stilted, formal writing style of the issue author, I was curious as to their motivation. It turns out that the primary activity of the https://github.com/iotamega GitHub account is opening this feature request and issue https://github.com/bitcoin/bitcoin/issues/29916. Per their sites https://x.com/Founders, https://bytesandburgers.com/about/ and h
...
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2380125677)
I've commented on this feature request at https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2380088608.
Given the insistence and somewhat stilted, formal writing style of the issue author, I was curious as to their motivation. It turns out that the primary activity of the https://github.com/iotamega GitHub account is opening this feature request and issue https://github.com/bitcoin/bitcoin/issues/29916. Per their sites https://x.com/Founders, https://bytesandburgers.com/about/ and h
...
💬 jonatack commented on pull request "doc: Minor update to doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/30992#discussion_r1779227724)
This change looks like a continuation of https://github.com/bitcoin/bitcoin/pull/14511.
Perhaps go a bit further, if we continue to prefer to give an idea of disk space and sync time here.
```suggestion
Bitcoin Core is the original Bitcoin client and it builds the backbone of the network. It downloads and, by default, stores the entire history of Bitcoin transactions, which requires several hundred gigabytes or more of disk space. Depending on the speed of your computer and network connec
...
(https://github.com/bitcoin/bitcoin/pull/30992#discussion_r1779227724)
This change looks like a continuation of https://github.com/bitcoin/bitcoin/pull/14511.
Perhaps go a bit further, if we continue to prefer to give an idea of disk space and sync time here.
```suggestion
Bitcoin Core is the original Bitcoin client and it builds the backbone of the network. It downloads and, by default, stores the entire history of Bitcoin transactions, which requires several hundred gigabytes or more of disk space. Depending on the speed of your computer and network connec
...
💬 Mackain commented on pull request "doc: Minor update to doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/30992#discussion_r1779237826)
@jonatack good point! thanks for the suggestion
(https://github.com/bitcoin/bitcoin/pull/30992#discussion_r1779237826)
@jonatack good point! thanks for the suggestion
🤔 mzumsande reviewed a pull request: "init: Remove retry for loop"
(https://github.com/bitcoin/bitcoin/pull/30968#pullrequestreview-2334717617)
Concept ACK
I think that one minor change in behavior is that with the old code we could retry many times if, for some reason, we'd repeatedly get `ChainstateLoadStatus::FAILURE` messages (which shouldn't happen normally, but might in some exotic failure scenarios?).
But even if it was possible to get these messages repeatedly, it doesn't really make sense to retry more than once.
(https://github.com/bitcoin/bitcoin/pull/30968#pullrequestreview-2334717617)
Concept ACK
I think that one minor change in behavior is that with the old code we could retry many times if, for some reason, we'd repeatedly get `ChainstateLoadStatus::FAILURE` messages (which shouldn't happen normally, but might in some exotic failure scenarios?).
But even if it was possible to get these messages repeatedly, it doesn't really make sense to retry more than once.
💬 mzumsande commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1779260055)
This log error is also shown in the non-GUI case, where it doesn't make any sense because the user didn't get the option to rebuild the block database. Also, to be pedantic, it's misleading in the GUI because we didn't abort a rebuild, but aborted instead of trying it. Maybe it's not necessary at all?
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1779260055)
This log error is also shown in the non-GUI case, where it doesn't make any sense because the user didn't get the option to rebuild the block database. Also, to be pedantic, it's misleading in the GUI because we didn't abort a rebuild, but aborted instead of trying it. Maybe it's not necessary at all?
💬 mzumsande commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1779222847)
nit: might use "retry with do_reindex set". Otherwise it might sounds a bit like "try turning it off and on again".
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1779222847)
nit: might use "retry with do_reindex set". Otherwise it might sounds a bit like "try turning it off and on again".
💬 ryanofsky commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2380253538)
> I think that one minor change in behavior is that with the old code we could retry many times if, for some reason, we'd repeatedly get `ChainstateLoadStatus::FAILURE` messages (which shouldn't happen normally, but might in some exotic failure scenarios?)
I think it wouldn't retry multiple times because the old code sets `do_reindex = true`
https://github.com/bitcoin/bitcoin/blob/d812cf11896a2214467b6fa72d7b763bac6077c5/src/init.cpp#L1643
At least that was my interpretation reviewing t
...
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2380253538)
> I think that one minor change in behavior is that with the old code we could retry many times if, for some reason, we'd repeatedly get `ChainstateLoadStatus::FAILURE` messages (which shouldn't happen normally, but might in some exotic failure scenarios?)
I think it wouldn't retry multiple times because the old code sets `do_reindex = true`
https://github.com/bitcoin/bitcoin/blob/d812cf11896a2214467b6fa72d7b763bac6077c5/src/init.cpp#L1643
At least that was my interpretation reviewing t
...
💬 jonatack commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2380259736)
In this context, concept ack with waiting for a more general solution rather than iterative piecemeal changes, as long as achieving it (via cluster mempool) looks reasonably attainable.
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2380259736)
In this context, concept ack with waiting for a more general solution rather than iterative piecemeal changes, as long as achieving it (via cluster mempool) looks reasonably attainable.
💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1779311123)
Since #30921, we can now make these `constexpr string_view`.
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1779311123)
Since #30921, we can now make these `constexpr string_view`.