🤔 rkrux reviewed a pull request: "rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures"
(https://github.com/bitcoin/bitcoin/pull/33014#pullrequestreview-3379921790)
Concept ACK 507501ddfa3f2c538a0817a5645b20ddde554362
> I don't think the corresponding bug is still present after v30, see: https://github.com/bitcoin/bitcoin/issues/32849#issuecomment-3443211089
I was able to reproduce this bug in the functional tests.
> Also, a test case in rpc_psbt.py would be good to verify the fix.
Consider adding the following test case in the second commit in this PR. The following test fails without this fix and passes with it.
```diff
diff --git a/test/functional/r
...
(https://github.com/bitcoin/bitcoin/pull/33014#pullrequestreview-3379921790)
Concept ACK 507501ddfa3f2c538a0817a5645b20ddde554362
> I don't think the corresponding bug is still present after v30, see: https://github.com/bitcoin/bitcoin/issues/32849#issuecomment-3443211089
I was able to reproduce this bug in the functional tests.
> Also, a test case in rpc_psbt.py would be good to verify the fix.
Consider adding the following test case in the second commit in this PR. The following test fails without this fix and passes with it.
```diff
diff --git a/test/functional/r
...
💬 rkrux commented on issue "Internal bug detected: FinalizeAndExtractPSBT(psbtx_copy, mtx)":
(https://github.com/bitcoin/bitcoin/issues/32849#issuecomment-3446588952)
Besides reproducing the cases mentioned in this issue via regtest, I was also able to reproduce the bug in functional tests and suggested adding it in the PR that contains the fix: https://github.com/bitcoin/bitcoin/pull/33014#pullrequestreview-3379921790
(https://github.com/bitcoin/bitcoin/issues/32849#issuecomment-3446588952)
Besides reproducing the cases mentioned in this issue via regtest, I was also able to reproduce the bug in functional tests and suggested adding it in the PR that contains the fix: https://github.com/bitcoin/bitcoin/pull/33014#pullrequestreview-3379921790
💬 maflcko commented on pull request "test: Use same rpc timeout for authproxy and cli":
(https://github.com/bitcoin/bitcoin/pull/33698#issuecomment-3446639814)
> I went through the new changes, built the code, ran the specific test p2p_headers_sync_with_minchainwork.py --timeout-factor=0.2 in both RPC and CLI modes and everything looks good .
It shouldn't look "good". They should both timeout with the same timeout value.
> but why did we decide on half the timeout for CLI instead of using the same value?
The goal of this pull is to use the exact same value, which is also what it does.
(https://github.com/bitcoin/bitcoin/pull/33698#issuecomment-3446639814)
> I went through the new changes, built the code, ran the specific test p2p_headers_sync_with_minchainwork.py --timeout-factor=0.2 in both RPC and CLI modes and everything looks good .
It shouldn't look "good". They should both timeout with the same timeout value.
> but why did we decide on half the timeout for CLI instead of using the same value?
The goal of this pull is to use the exact same value, which is also what it does.
⚠️ waketraindev opened an issue: "qt: rpc console, filtered commands replaced with '(…)' may execute unintended actions when recalled from history"
(https://github.com/bitcoin-core/gui/issues/907)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
In the Qt rpc console, when a command is filtered for privacy (`historyFilter`) it's parameters are replaced with `(…)`
For example:
```
createwallet "testwallet"
recalled as: createwallet(…)
```
If this filtered command is later recalled from command history (ArrowUp+Enter), it executes literally, creating or
executing an unintended command such as `createwallet(…)`
This will then cre
...
(https://github.com/bitcoin-core/gui/issues/907)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
In the Qt rpc console, when a command is filtered for privacy (`historyFilter`) it's parameters are replaced with `(…)`
For example:
```
createwallet "testwallet"
recalled as: createwallet(…)
```
If this filtered command is later recalled from command history (ArrowUp+Enter), it executes literally, creating or
executing an unintended command such as `createwallet(…)`
This will then cre
...
💬 maflcko commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2462779173)
This is a no-op. You assign the value and the expected value from the same function, and then assert that it is equal. This changes what the test is checking.
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2462779173)
This is a no-op. You assign the value and the expected value from the same function, and then assert that it is equal. This changes what the test is checking.
💬 yancyribbens commented on pull request "test: add case where `TOTAL_TRIES` is exceeded yet solution remains":
(https://github.com/bitcoin/bitcoin/pull/33701#discussion_r2462796228)
Thanks for taking a look. The name doppelgangers comes from a similar test for BnB: https://github.com/bitcoin/bitcoin/blob/e744fd1249bf9577274614eaf3997bf4bbb612ff/src/wallet/test/coinselection_tests.cpp#L166.
I'm impartial on `res_a` vs `result_a`. I couldn't name it just `res` because there would be multiple assignments to `res` which the compiler complains about.
(https://github.com/bitcoin/bitcoin/pull/33701#discussion_r2462796228)
Thanks for taking a look. The name doppelgangers comes from a similar test for BnB: https://github.com/bitcoin/bitcoin/blob/e744fd1249bf9577274614eaf3997bf4bbb612ff/src/wallet/test/coinselection_tests.cpp#L166.
I'm impartial on `res_a` vs `result_a`. I couldn't name it just `res` because there would be multiple assignments to `res` which the compiler complains about.
💬 hMsats commented on issue "Seemingly second (very long) validation at the same height":
(https://github.com/bitcoin/bitcoin/issues/33687#issuecomment-3446821485)
I'm now running/testing my node (starting cold) with:
```
std::chrono::seconds{consensusParams.nPowTargetSpacing}/10 * (BLOCK_DOWNLOAD_TIMEOUT_BASE + BLOCK_DOWNLOAD_TIMEOUT_PER_PEER * nOtherPeersWithValidatedDownloads))
```
in `net_processing.cpp`, where I introduced the division by a factor of 10. So based on 1 minute timeout instead of 10 minutes. When my node was warm, I could even base it on a 6 seconds timeout without problems (I've tested with 1 second to see if the timeout message was vi
...
(https://github.com/bitcoin/bitcoin/issues/33687#issuecomment-3446821485)
I'm now running/testing my node (starting cold) with:
```
std::chrono::seconds{consensusParams.nPowTargetSpacing}/10 * (BLOCK_DOWNLOAD_TIMEOUT_BASE + BLOCK_DOWNLOAD_TIMEOUT_PER_PEER * nOtherPeersWithValidatedDownloads))
```
in `net_processing.cpp`, where I introduced the division by a factor of 10. So based on 1 minute timeout instead of 10 minutes. When my node was warm, I could even base it on a 6 seconds timeout without problems (I've tested with 1 second to see if the timeout message was vi
...
🤔 furszy reviewed a pull request: "interfaces: enable cancelling running `waitNext` calls"
(https://github.com/bitcoin/bitcoin/pull/33676#pullrequestreview-3380105952)
Concept ACK. Will review soon.
(https://github.com/bitcoin/bitcoin/pull/33676#pullrequestreview-3380105952)
Concept ACK. Will review soon.
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3446886770)
rfm?
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3446886770)
rfm?
🤔 hebasto reviewed a pull request: "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2"
(https://github.com/bitcoin/bitcoin/pull/33185#pullrequestreview-3380137916)
My Guix build:
```
x86_64
3de417ac1dade848d8b1609adf72c0faefcaf5acf03199259aa3aeb83e6a863f guix-build-59c4898994bd/output/aarch64-linux-gnu/SHA256SUMS.part
c6eda45fce2b34940ea53caa2acf0c1436122f89b85c0bc727834360f78a40f5 guix-build-59c4898994bd/output/aarch64-linux-gnu/bitcoin-59c4898994bd-aarch64-linux-gnu-debug.tar.gz
41584667134bfc5c35851d5005692c447d3fc529a310c7868030d6383d8bd840 guix-build-59c4898994bd/output/aarch64-linux-gnu/bitcoin-59c4898994bd-aarch64-linux-gnu.tar.gz
5c6d784bc03e59c
...
(https://github.com/bitcoin/bitcoin/pull/33185#pullrequestreview-3380137916)
My Guix build:
```
x86_64
3de417ac1dade848d8b1609adf72c0faefcaf5acf03199259aa3aeb83e6a863f guix-build-59c4898994bd/output/aarch64-linux-gnu/SHA256SUMS.part
c6eda45fce2b34940ea53caa2acf0c1436122f89b85c0bc727834360f78a40f5 guix-build-59c4898994bd/output/aarch64-linux-gnu/bitcoin-59c4898994bd-aarch64-linux-gnu-debug.tar.gz
41584667134bfc5c35851d5005692c447d3fc529a310c7868030d6383d8bd840 guix-build-59c4898994bd/output/aarch64-linux-gnu/bitcoin-59c4898994bd-aarch64-linux-gnu.tar.gz
5c6d784bc03e59c
...
👍 hebasto approved a pull request: "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2"
(https://github.com/bitcoin/bitcoin/pull/33185#pullrequestreview-3380138604)
re-ACK 59c4898994bde3d86168075f0031c9d5a9ac5c8f.
(https://github.com/bitcoin/bitcoin/pull/33185#pullrequestreview-3380138604)
re-ACK 59c4898994bde3d86168075f0031c9d5a9ac5c8f.
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2462960057)
Thanks for the review @maflcko! You were spot on, I'd turned that into a no-op.
I've restored the gen_test_data / load_test_data logic so it's a proper regression test again. I've generated the new rpc_getblockstats.json file and added it to the PR.
Also updated the PR description to better reflect changes made
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2462960057)
Thanks for the review @maflcko! You were spot on, I'd turned that into a no-op.
I've restored the gen_test_data / load_test_data logic so it's a proper regression test again. I've generated the new rpc_getblockstats.json file and added it to the PR.
Also updated the PR description to better reflect changes made
💬 hebasto commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-3446975081)
> > > Yes. I forgot to put this link into my previous comment: [bitcoin-core/secp256k1#1681](https://github.com/bitcoin-core/secp256k1/pull/1681).
> >
> >
> > Have you seen the same from actually running Core, or it's benchmarks, or an IBD?
>
> I'll refresh my benchmarks and post them shortly.
Below are benchmarks on my Windows machine:
<details>
<summary>Compiled with MSVC</summary>
```
| ns/op | op/s | err% | total | benchmark
|----------
...
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-3446975081)
> > > Yes. I forgot to put this link into my previous comment: [bitcoin-core/secp256k1#1681](https://github.com/bitcoin-core/secp256k1/pull/1681).
> >
> >
> > Have you seen the same from actually running Core, or it's benchmarks, or an IBD?
>
> I'll refresh my benchmarks and post them shortly.
Below are benchmarks on my Windows machine:
<details>
<summary>Compiled with MSVC</summary>
```
| ns/op | op/s | err% | total | benchmark
|----------
...
💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2462992380)
done in https://github.com/bitcoin/bitcoin/pull/24539/commits/9d3dfd77224fbe40033feccb346a167dc5f031a7
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2462992380)
done in https://github.com/bitcoin/bitcoin/pull/24539/commits/9d3dfd77224fbe40033feccb346a167dc5f031a7
💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2462992391)
done in https://github.com/bitcoin/bitcoin/pull/24539/commits/9d3dfd77224fbe40033feccb346a167dc5f031a7
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2462992391)
done in https://github.com/bitcoin/bitcoin/pull/24539/commits/9d3dfd77224fbe40033feccb346a167dc5f031a7
💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2462992401)
done in https://github.com/bitcoin/bitcoin/pull/24539/commits/9d3dfd77224fbe40033feccb346a167dc5f031a7
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2462992401)
done in https://github.com/bitcoin/bitcoin/pull/24539/commits/9d3dfd77224fbe40033feccb346a167dc5f031a7
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3447786473)
Tested the performance of the new REST endpoint using `ab` on mainnet with various `SIZE` parameters:
```
$ BLOCKHASH=00000000000000000001eae3683a5350b67ddb17d9c7b6c8010ab5b36ccbaa09
$ ab -q -k -c 1 -n 100000 'http://localhost:8332/rest/blockpart/BLOCKHASH.bin?offset=0&size=SIZE'
```
| `SIZE` | Time per request [ms] |
| - | - |
| 1 | 0.03 |
| 10 | 0.03 |
| 100 | 0.03 |
| 1k | 0.03 |
| 10k | 0.04 |
| 100k | 0.07 |
| 1M | 0.6 |
So fetching a typical transaction is expected to tak
...
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3447786473)
Tested the performance of the new REST endpoint using `ab` on mainnet with various `SIZE` parameters:
```
$ BLOCKHASH=00000000000000000001eae3683a5350b67ddb17d9c7b6c8010ab5b36ccbaa09
$ ab -q -k -c 1 -n 100000 'http://localhost:8332/rest/blockpart/BLOCKHASH.bin?offset=0&size=SIZE'
```
| `SIZE` | Time per request [ms] |
| - | - |
| 1 | 0.03 |
| 10 | 0.03 |
| 100 | 0.03 |
| 1k | 0.03 |
| 10k | 0.04 |
| 100k | 0.07 |
| 1M | 0.6 |
So fetching a typical transaction is expected to tak
...
💬 andrewtoth commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3447826732)
Compared your command fetching SIZE=788 and block 850000 with a 788 byte tx in that block 2d2dcc80195541dd44209dcfeb25393c8c8710f262360368a618b7ff3fa3f08c using `getrawtransaction` with `txindex`. I used hex encoding to make the comparison more fair.
```
ab -q -k -c 1 -n 1000000 'http://localhost:8332/rest/blockpart/00000000000000000002a0b5db2a7f8d9087464c2586b546be7bce8eb53b8187.hex?offset=0&size=788'
```
I got `Time per request: 0.037 [ms] (mean)`.
```
ab -q -k -c 1 -n 1000000 -p
...
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3447826732)
Compared your command fetching SIZE=788 and block 850000 with a 788 byte tx in that block 2d2dcc80195541dd44209dcfeb25393c8c8710f262360368a618b7ff3fa3f08c using `getrawtransaction` with `txindex`. I used hex encoding to make the comparison more fair.
```
ab -q -k -c 1 -n 1000000 'http://localhost:8332/rest/blockpart/00000000000000000002a0b5db2a7f8d9087464c2586b546be7bce8eb53b8187.hex?offset=0&size=788'
```
I got `Time per request: 0.037 [ms] (mean)`.
```
ab -q -k -c 1 -n 1000000 -p
...
📝 redmsqt opened a pull request: "Add base fileBaseb"
(https://github.com/bitcoin/bitcoin/pull/33703)
<!--
*** 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/33703)
<!--
*** 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
...
💬 redmsqt commented on pull request "Add base fileBaseb":
(https://github.com/bitcoin/bitcoin/pull/33703#issuecomment-3447957799)
Base
(https://github.com/bitcoin/bitcoin/pull/33703#issuecomment-3447957799)
Base
📝 mccoyadd opened a pull request: "Add CI workflow for C/C++ projects"
(https://github.com/bitcoin/bitcoin/pull/33704)
<!--
*** 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/33704)
<!--
*** 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
...