π¬ m3dwards commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167559798)
Changed to `--target-dir`.
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167559798)
Changed to `--target-dir`.
π¬ fjahr commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3006057356)
Code review ACK d06942c6731d5db7326bc565655b33a379a5d9b0
(https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3006057356)
Code review ACK d06942c6731d5db7326bc565655b33a379a5d9b0
π¬ glozow commented on pull request "wallet, rpc: Remove deprecated balances from getwalletinfo and getunconfirmedbalance":
(https://github.com/bitcoin/bitcoin/pull/32721#issuecomment-3006074516)
Not required, but I think it would be nice to write a release note mentioning "these fields from `getwalletinfo` which were deprecated in 0.19 are now unavailable. Here are the equivalent fields from `getbalances`: etc etc etc"
(https://github.com/bitcoin/bitcoin/pull/32721#issuecomment-3006074516)
Not required, but I think it would be nice to write a release note mentioning "these fields from `getwalletinfo` which were deprecated in 0.19 are now unavailable. Here are the equivalent fields from `getbalances`: etc etc etc"
π¬ hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2167578566)
Renumbered in the "Improve comments"-commit.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2167578566)
Renumbered in the "Improve comments"-commit.
π¬ hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2167580914)
Broke out `bool buffer_exceeded`, let me know if you think it's an improvement.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2167580914)
Broke out `bool buffer_exceeded`, let me know if you think it's an improvement.
π¬ hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2167525791)
No, rebase mishap. Thanks for catching!
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2167525791)
No, rebase mishap. Thanks for catching!
π¬ hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2167506799)
Slightly more confusing error, but like that it's more terse. Taken.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2167506799)
Slightly more confusing error, but like that it's more terse. Taken.
π¬ hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2167541369)
Agree. New attempt:
```C++
//! Only start outputting headers once this many headers have been received
//! and validated against commitments.
```
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2167541369)
Agree. New attempt:
```C++
//! Only start outputting headers once this many headers have been received
//! and validated against commitments.
```
π glozow merged a pull request: "wallet, rpc: Remove deprecated balances from getwalletinfo and getunconfirmedbalance"
(https://github.com/bitcoin/bitcoin/pull/32721)
(https://github.com/bitcoin/bitcoin/pull/32721)
π hodlinator approved a pull request: "test: enabling wallet migration functional test on windows"
(https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2959616863)
re-ACK 941b8f54c0d35d3243bb6083f3b52681d1b9a555
Fixed some nits (https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2959123050) since my first ACK (https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2921071060).
Passed linter locally.
(https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2959616863)
re-ACK 941b8f54c0d35d3243bb6083f3b52681d1b9a555
Fixed some nits (https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2959123050) since my first ACK (https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2921071060).
Passed linter locally.
π¬ l0rinc commented on pull request "merkle: preβreserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-3006209711)
I've rebased the changed and adjusted the benchmark to be more similar to the other production usages in the first commit, rounded to even in the second (optimization) commit, so that we can realistically measure the speed difference before and after:
% build/bin/bench_bitcoin -filter='MerkleRoot' --min-time=1000
Before 7f620cffebee593e48434cf182cc2fd64a6d76be:
| ns/leaf | leaf/s | err% | total | benchmark
|--------------------:|--------------------:|---
...
(https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-3006209711)
I've rebased the changed and adjusted the benchmark to be more similar to the other production usages in the first commit, rounded to even in the second (optimization) commit, so that we can realistically measure the speed difference before and after:
% build/bin/bench_bitcoin -filter='MerkleRoot' --min-time=1000
Before 7f620cffebee593e48434cf182cc2fd64a6d76be:
| ns/leaf | leaf/s | err% | total | benchmark
|--------------------:|--------------------:|---
...
π solveforceapp opened a pull request: "Codex/explore paradoxes in language and coding"
(https://github.com/bitcoin/bitcoin/pull/32815)
<!--
*** 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
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/32815)
<!--
*** 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
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
π¬ darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3006215185)
> I have a preference for breaking out the new checks into its own function since if we intend it to someday soon(TM) be a consensus rule it would have to be pulled out anyways. [instagibbs@5d63372](https://github.com/instagibbs/bitcoin/commit/5d63372f75c4a2403c4032b63e5c604ee96c5a40)
If we do this the separate function could be much simpler than that (no solver etc). I'll have a go at it tomorrow morning, inspired by the consensus implementation i have on a private branch.
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3006215185)
> I have a preference for breaking out the new checks into its own function since if we intend it to someday soon(TM) be a consensus rule it would have to be pulled out anyways. [instagibbs@5d63372](https://github.com/instagibbs/bitcoin/commit/5d63372f75c4a2403c4032b63e5c604ee96c5a40)
If we do this the separate function could be much simpler than that (no solver etc). I'll have a go at it tomorrow morning, inspired by the consensus implementation i have on a private branch.
β
achow101 closed a pull request: "Codex/explore paradoxes in language and coding"
(https://github.com/bitcoin/bitcoin/pull/32815)
(https://github.com/bitcoin/bitcoin/pull/32815)
π theStack approved a pull request: "doc: Add fetching single PRs from upstream to productivity.md"
(https://github.com/bitcoin/bitcoin/pull/32783#pullrequestreview-2959753896)
ACK 45b1d39757668939b03b27401c324a938ef0cd8d
(https://github.com/bitcoin/bitcoin/pull/32783#pullrequestreview-2959753896)
ACK 45b1d39757668939b03b27401c324a938ef0cd8d
π¬ l0rinc commented on pull request "clang-format: modernize and realign clang-format configuration":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2167702132)
I've added all of them, this was the simplest change with fewest commits and explanations
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2167702132)
I've added all of them, this was the simplest change with fewest commits and explanations
π€ pablomartin4btc reviewed a pull request: "test: fix catchup loop in outbound eviction functional test"
(https://github.com/bitcoin/bitcoin/pull/32742#pullrequestreview-2959781264)
cr-ACK dd8447f70faf6419b4617da3c1b57098e9cd66a6
As I verified, `tip_header` was never updated since the top of the test and `prev_prev_hash` had the wrong value assigned (`tip_header.hash` which was `None`), but the test was passing due to `wait_for_getheaders` when the `block_hash` is `None` (same behaviour as if nothing is passed to the function), [checks](https://github.com/bitcoin/bitcoin/blob/f27898c8bfe339e2cad09d42e133a05602b1a030/test/functional/test_framework/p2p.py#L667-L669) for la
...
(https://github.com/bitcoin/bitcoin/pull/32742#pullrequestreview-2959781264)
cr-ACK dd8447f70faf6419b4617da3c1b57098e9cd66a6
As I verified, `tip_header` was never updated since the top of the test and `prev_prev_hash` had the wrong value assigned (`tip_header.hash` which was `None`), but the test was passing due to `wait_for_getheaders` when the `block_hash` is `None` (same behaviour as if nothing is passed to the function), [checks](https://github.com/bitcoin/bitcoin/blob/f27898c8bfe339e2cad09d42e133a05602b1a030/test/functional/test_framework/p2p.py#L667-L669) for la
...
π€ glozow reviewed a pull request: "test: fix an incorrect `feature_fee_estimation.py` subtest"
(https://github.com/bitcoin/bitcoin/pull/32463#pullrequestreview-2959775440)
Nice work finding the issue. For some reason, I couldn't reproduce using `--random=3450808900320758527`. But `--random=4130785039185616282` from https://cirrus-ci.com/task/5487636053229568 and `--random=6565824249277094228` from https://github.com/bitcoin/bitcoin/runs/43526123006 worked for me.
Did you consider baking this logic into `check_smart_estimates` instead? It feels a bit hacky to just append it to the fees seen, given that it isn't actually a seen fee.
```
diff --git a/test/func
...
(https://github.com/bitcoin/bitcoin/pull/32463#pullrequestreview-2959775440)
Nice work finding the issue. For some reason, I couldn't reproduce using `--random=3450808900320758527`. But `--random=4130785039185616282` from https://cirrus-ci.com/task/5487636053229568 and `--random=6565824249277094228` from https://github.com/bitcoin/bitcoin/runs/43526123006 worked for me.
Did you consider baking this logic into `check_smart_estimates` instead? It feels a bit hacky to just append it to the fees seen, given that it isn't actually a seen fee.
```
diff --git a/test/func
...
π¬ glozow commented on pull request "test: fix an incorrect `feature_fee_estimation.py` subtest":
(https://github.com/bitcoin/bitcoin/pull/32463#discussion_r2167700984)
Agree this naming is better π
(https://github.com/bitcoin/bitcoin/pull/32463#discussion_r2167700984)
Agree this naming is better π
π glozow merged a pull request: "test: fix catchup loop in outbound eviction functional test"
(https://github.com/bitcoin/bitcoin/pull/32742)
(https://github.com/bitcoin/bitcoin/pull/32742)