🚀 fanquake merged a pull request: "test: Detect truncated download in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/34040)
(https://github.com/bitcoin/bitcoin/pull/34040)
💬 andrewtoth commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2607449049)
4564e571f2ef22ac096c58033137154e1e4b4cf7
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2607449049)
4564e571f2ef22ac096c58033137154e1e4b4cf7
💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607459246)
Not all of the text is by my own judgement. However, you might leave out BIP-32 derivations for keys not under own management. Technically, those were never expected to be signed anyways.
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607459246)
Not all of the text is by my own judgement. However, you might leave out BIP-32 derivations for keys not under own management. Technically, those were never expected to be signed anyways.
💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3638047678)
ACK 8b417087aec4671a1ce58f2331d1688e665d9935
The implementation doesn't slow down the critical path anymore and is beautifully isolated from the rest of the raw block reading.
The tests contain the extremes (added since last ACK), docs were update, braces added.
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3638047678)
ACK 8b417087aec4671a1ce58f2331d1688e665d9935
The implementation doesn't slow down the critical path anymore and is beautifully isolated from the rest of the raw block reading.
The tests contain the extremes (added since last ACK), docs were update, braces added.
💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607375727)
It came back, as expected, it shows the benchmark getting slower because of the vector recreation (making it more realistic):
<img width="711" height="121" alt="image" src="https://github.com/user-attachments/assets/acb14880-0371-4a8a-910e-5e04bbf79aa3" />
---
I have checked that the refactor doesn't introduce any regression now 👍
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607375727)
It came back, as expected, it shows the benchmark getting slower because of the vector recreation (making it more realistic):
<img width="711" height="121" alt="image" src="https://github.com/user-attachments/assets/acb14880-0371-4a8a-910e-5e04bbf79aa3" />
---
I have checked that the refactor doesn't introduce any regression now 👍
💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607453751)
nit: the above test makes res a `REQUIRE`, if you touch agan, consider unifying. Definitely not a blocker.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607453751)
nit: the above test makes res a `REQUIRE`, if you touch agan, consider unifying. Definitely not a blocker.
💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607445185)
@romanz, can you please check if there's a performance difference in the RPC with and without a const here?
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607445185)
@romanz, can you please check if there's a performance difference in the RPC with and without a const here?
💬 mzumsande commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2607462801)
nothing for this PR, but since these things are quite common (recently #33121), I wonder if a mode `-test=cutoffexponentialtimers` or something similar would make sense which would cut off the long tail of all exponential timers (`rand_exp_duration`) at some percentile, so we don't need to deal with these probabilistic failures in functional tests.
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2607462801)
nothing for this PR, but since these things are quite common (recently #33121), I wonder if a mode `-test=cutoffexponentialtimers` or something similar would make sense which would cut off the long tail of all exponential timers (`rand_exp_duration`) at some percentile, so we don't need to deal with these probabilistic failures in functional tests.
💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607469350)
I took those from #33947. I'll tweak them a bit.
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607469350)
I took those from #33947. I'll tweak them a bit.
💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2607508978)
this is defintely too verbose, but some some explanation for the getters would be welcome
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2607508978)
this is defintely too verbose, but some some explanation for the getters would be welcome
💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2607517231)
```suggestion
if (!IsDirty()) return;
```
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2607517231)
```suggestion
if (!IsDirty()) return;
```
💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2607512445)
nit: if you think it makes it more readable
```suggestion
bool IsDirty() const noexcept { return !!m_next; }
```
or
```suggestion
bool IsDirty() const noexcept { return m_next != nullptr; }
```
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2607512445)
nit: if you think it makes it more readable
```suggestion
bool IsDirty() const noexcept { return !!m_next; }
```
or
```suggestion
bool IsDirty() const noexcept { return m_next != nullptr; }
```
💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607533987)
Not so. Not all commands are expected to interact via stdin. `signtx` does interact via stdin. I removed another sections, but left on the "On success ..". Based on what is it verbose, because I am not otherwise familiar with Bitcoin Core RPC and I find it useful to have these bits of behavior made explicit.
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607533987)
Not so. Not all commands are expected to interact via stdin. `signtx` does interact via stdin. I removed another sections, but left on the "On success ..". Based on what is it verbose, because I am not otherwise familiar with Bitcoin Core RPC and I find it useful to have these bits of behavior made explicit.
💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607537587)
My involvement is not through HWI. I want to emphasize that presence of `error` in a successful result is not an issue if `"error": null`.
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607537587)
My involvement is not through HWI. I want to emphasize that presence of `error` in a successful result is not an issue if `"error": null`.
💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607540719)
Dropped this, and cleaning up other parts. This is messy because the documentation describes both the commandline interface and the stdin at the same time.
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607540719)
Dropped this, and cleaning up other parts. This is messy because the documentation describes both the commandline interface and the stdin at the same time.
👍 brunoerg approved a pull request: "fuzz: Add tests for `CCoinControl` methods"
(https://github.com/bitcoin/bitcoin/pull/34026#pullrequestreview-3563662802)
code review ACK 31904f1ba1de49890f605f1d7c907790780831ec
It's fine as is, but I think we could improve this harness a little bit by adding some assertions. Some quite simple examples:
```diff
@@ -87,9 +87,7 @@ FUZZ_TARGET(coincontrol, .init = initialize_coincontrol)
[&] {
uint32_t sequence{fuzzed_data_provider.ConsumeIntegral<uint32_t>()};
(void)coin_control.Select(out_point).SetSequence(sequence);
- },
- [&] {
-
...
(https://github.com/bitcoin/bitcoin/pull/34026#pullrequestreview-3563662802)
code review ACK 31904f1ba1de49890f605f1d7c907790780831ec
It's fine as is, but I think we could improve this harness a little bit by adding some assertions. Some quite simple examples:
```diff
@@ -87,9 +87,7 @@ FUZZ_TARGET(coincontrol, .init = initialize_coincontrol)
[&] {
uint32_t sequence{fuzzed_data_provider.ConsumeIntegral<uint32_t>()};
(void)coin_control.Select(out_point).SetSequence(sequence);
- },
- [&] {
-
...
💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607546750)
Dropping.
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607546750)
Dropping.
💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607553947)
Nope. I do not have sufficient knowledge of Bitcoin Core to state this. Do you have an idea? My goal is to emphasize that older versions behaved differently and less refined, such that there is benefit to start using external-signers with recent Bitcoin versions.
Maybe we should just leave it out?
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607553947)
Nope. I do not have sufficient knowledge of Bitcoin Core to state this. Do you have an idea? My goal is to emphasize that older versions behaved differently and less refined, such that there is benefit to start using external-signers with recent Bitcoin versions.
Maybe we should just leave it out?
💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607555778)
Copied from other PR, to include their improvements. I'll tweak it.
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607555778)
Copied from other PR, to include their improvements. I'll tweak it.
💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#discussion_r2607580627)
It shouldn't make any difference, for building host tools. It also seems non-trivial to swap out the version in `gcc-toolchain-14`.
(https://github.com/bitcoin/bitcoin/pull/33775#discussion_r2607580627)
It shouldn't make any difference, for building host tools. It also seems non-trivial to swap out the version in `gcc-toolchain-14`.