💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778882653)
We could add the examples from `bitcoin-cli` here:
```C++
BOOST_AUTO_TEST_CASE(ConstevalFormatString_SpecificUsageTests)
{
// These format strings contain '*' and should skip validation
PassFmt<6>("<-> type net v mping ping send recv txn blk hb %*s%*s%*s ");
PassFmt<5>("%*s %-*s%s\n");
PassFmt<22>(
"%3s %6s %5s %2s%7s%7s%5s%5s%5s%5s %2s %*s%*s%*s%*i %*s %-*s%s\n");
PassFmt<2>(" ms ms sec sec min min %*
...
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778882653)
We could add the examples from `bitcoin-cli` here:
```C++
BOOST_AUTO_TEST_CASE(ConstevalFormatString_SpecificUsageTests)
{
// These format strings contain '*' and should skip validation
PassFmt<6>("<-> type net v mping ping send recv txn blk hb %*s%*s%*s ");
PassFmt<5>("%*s %-*s%s\n");
PassFmt<22>(
"%3s %6s %5s %2s%7s%7s%5s%5s%5s%5s %2s %*s%*s%*s%*i %*s %-*s%s\n");
PassFmt<2>(" ms ms sec sec min min %*
...
💬 0xB10C commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2379684081)
Played around with the `getorphantxs 1` output a bit regarding visualization. You can now plug your own `getorphantxs.json` (verbosity >=1) into this demo tool https://observablehq.com/d/fb8142e7eb7f98f3. Produces something like this based on the size and from-peers (color) of the orphans.
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2379684081)
Played around with the `getorphantxs 1` output a bit regarding visualization. You can now plug your own `getorphantxs.json` (verbosity >=1) into this demo tool https://observablehq.com/d/fb8142e7eb7f98f3. Produces something like this based on the size and from-peers (color) of the orphans.
💬 theuni commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778899716)
To avoid a race, this needs to lock `kernel_notifications.m_tip_block_mutex` before calling notify. `(*node.shutdown_signal)()` doesn't necessarily need to happen under the lock, but a lock is required after the signal in order to synchronize. Otherwise the update could be missed and we could potentially wait forever.
This is currently an example of [this trap](https://www.modernescpp.com/index.php/c-core-guidelines-be-aware-of-the-traps-of-condition-variables/) (see the "An atomic predicate"
...
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778899716)
To avoid a race, this needs to lock `kernel_notifications.m_tip_block_mutex` before calling notify. `(*node.shutdown_signal)()` doesn't necessarily need to happen under the lock, but a lock is required after the signal in order to synchronize. Otherwise the update could be missed and we could potentially wait forever.
This is currently an example of [this trap](https://www.modernescpp.com/index.php/c-core-guidelines-be-aware-of-the-traps-of-condition-variables/) (see the "An atomic predicate"
...
💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2379691000)
Here's an initial sketch of making Sv2Connman a subclass of SockMan. The test gets through the handshake but fails later on, so I'll need to study it a bit more closely.
https://github.com/Sjors/bitcoin/pull/64
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2379691000)
Here's an initial sketch of making Sv2Connman a subclass of SockMan. The test gets through the handshake but fails later on, so I'll need to study it a bit more closely.
https://github.com/Sjors/bitcoin/pull/64
💬 stickies-v commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778939114)
`"Copyright (C) %i-%i"` remains the translateable string both before and after this change. The only difference is that we use the `bilingual_str` `format` overload instead of the `const std::string&` one.
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778939114)
`"Copyright (C) %i-%i"` remains the translateable string both before and after this change. The only difference is that we use the `bilingual_str` `format` overload instead of the `const std::string&` one.
👍 theuni approved a pull request: "guix: Drop no longer needed `PATH` modification"
(https://github.com/bitcoin/bitcoin/pull/30989#pullrequestreview-2334276129)
utACK f1daa80521eccebe86af6ee6fa594edf40eaa676 since guix is happy.
> 2. I don't understand why "In the Guix environment, `${BASEPREFIX}/${HOST}/native/bin` is added to the `PATH` environment variable," according to the description. Setting this seems indiscriminate, like a sledgehammer approach, something that would cause the guix build to behave differently from normal depends builds and lead to confusing issues like this one.
As @hebasto mentioned [here](https://github.com/bitcoin/bitco
...
(https://github.com/bitcoin/bitcoin/pull/30989#pullrequestreview-2334276129)
utACK f1daa80521eccebe86af6ee6fa594edf40eaa676 since guix is happy.
> 2. I don't understand why "In the Guix environment, `${BASEPREFIX}/${HOST}/native/bin` is added to the `PATH` environment variable," according to the description. Setting this seems indiscriminate, like a sledgehammer approach, something that would cause the guix build to behave differently from normal depends builds and lead to confusing issues like this one.
As @hebasto mentioned [here](https://github.com/bitcoin/bitco
...
💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1778950526)
Done.
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1778950526)
Done.
💬 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
...