💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967413387)
thx, fixed
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967413387)
thx, fixed
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967413539)
thx, fixed
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967413539)
thx, fixed
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967413630)
thx, done
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967413630)
thx, done
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967413737)
I don't like the manual `\n` handling and the manual `\` handling. An alternative would be a string which natively contains newlines. However, I don't think https://clang.llvm.org/extra/clang-tidy/checks/performance/avoid-endl.html applies, because this isn't performance critical. Leaving as-is for now.
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967413737)
I don't like the manual `\n` handling and the manual `\` handling. An alternative would be a string which natively contains newlines. However, I don't think https://clang.llvm.org/extra/clang-tidy/checks/performance/avoid-endl.html applies, because this isn't performance critical. Leaving as-is for now.
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967413984)
You are right that the use of `assert` here may be a bit wrong to sanitize an input string. However, in the context of tests (or dev-only test-only scripts), the distinction doesn't matter, as long as any failure happens. (This is also similar of how the functional tests simply do an `assert` on the return code)
In any case, if this was changed, it should be applied to almost all `unwrap`,`expect`,or `panic` as well? For example, I could imagine that `expect("test failed")` could fail when th
...
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967413984)
You are right that the use of `assert` here may be a bit wrong to sanitize an input string. However, in the context of tests (or dev-only test-only scripts), the distinction doesn't matter, as long as any failure happens. (This is also similar of how the functional tests simply do an `assert` on the return code)
In any case, if this was changed, it should be applied to almost all `unwrap`,`expect`,or `panic` as well? For example, I could imagine that `expect("test failed")` could fail when th
...
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967414091)
Thx, done
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967414091)
Thx, done
💬 hodlinator commented on issue "qa: timeout in StopHTTPServer()":
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2678077496)
Spent a 2-3 days last week experimenting with solutions, ended up with:
- Improving docs.
- Fixing an adjacent memory leak.
- Improving debug-ability of future occurrences of this issue.
See #31929.
Looking forward to not relying in libevent in the future! Much nicer to be able to debug our code directly.
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2678077496)
Spent a 2-3 days last week experimenting with solutions, ended up with:
- Improving docs.
- Fixing an adjacent memory leak.
- Improving debug-ability of future occurrences of this issue.
See #31929.
Looking forward to not relying in libevent in the future! Much nicer to be able to debug our code directly.
👋 rkrux's pull request is ready for review: "test: add coverage for abandoning unconfirmed transaction"
(https://github.com/bitcoin/bitcoin/pull/31943)
(https://github.com/bitcoin/bitcoin/pull/31943)
💬 rkrux commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#issuecomment-2678157719)
Re: https://github.com/bitcoin/bitcoin/pull/31794#pullrequestreview-2605174936
I created a PR for this here https://github.com/bitcoin/bitcoin/pull/31943 and made @Eunovo co-author.
(https://github.com/bitcoin/bitcoin/pull/31794#issuecomment-2678157719)
Re: https://github.com/bitcoin/bitcoin/pull/31794#pullrequestreview-2605174936
I created a PR for this here https://github.com/bitcoin/bitcoin/pull/31943 and made @Eunovo co-author.
📝 maflcko reopened a pull request: "fuzz: Speed up *_package_eval fuzz targets a bit"
(https://github.com/bitcoin/bitcoin/pull/31457)
Each target is at least 10% faster for me when running over the current set of qa-assets, which seems nice.
(https://github.com/bitcoin/bitcoin/pull/31457)
Each target is at least 10% faster for me when running over the current set of qa-assets, which seems nice.
💬 maflcko commented on pull request "fuzz: Speed up *_package_eval fuzz targets a bit":
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1967477525)
Reopened this pull request and dropped the changes to `mempool_outpoints` for now.
I've kept the changes to `outpoints_value`, because:
* the map is only used for lookup, so the order should not matter
* It still provides a speedup of about 10%, albeit a bit less than the previous version
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1967477525)
Reopened this pull request and dropped the changes to `mempool_outpoints` for now.
I've kept the changes to `outpoints_value`, because:
* the map is only used for lookup, so the order should not matter
* It still provides a speedup of about 10%, albeit a bit less than the previous version
👋 maflcko's pull request is ready for review: "fuzz: Speed up *_package_eval fuzz targets a bit"
(https://github.com/bitcoin/bitcoin/pull/31457)
(https://github.com/bitcoin/bitcoin/pull/31457)
⚠️ maflcko opened an issue: "Intermittent failure in feature_fee_estimation.py in check_raw_estimates feerate = float(e["feerate"]) KeyError: 'feerate'"
(https://github.com/bitcoin/bitcoin/issues/31944)
( Reminds me of https://github.com/bitcoin/bitcoin/issues/30640 )
https://cirrus-ci.com/task/5862559074484224?logs=ci#L2975:
```
node1 2025-02-21T16:00:41.934000Z [httpworker.9] [rpc/request.cpp:241] [parse] [rpc] ThreadRPCServer method=syncwithvalidationinterfacequeue user=__cookie__
node2 2025-02-21T16:00:41.935013Z [http] [httpserver.cpp:306] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:34802
node2 2025-02-21T16:00:41.935352Z [httpworker.13] [rpc/request.cpp:24
...
(https://github.com/bitcoin/bitcoin/issues/31944)
( Reminds me of https://github.com/bitcoin/bitcoin/issues/30640 )
https://cirrus-ci.com/task/5862559074484224?logs=ci#L2975:
```
node1 2025-02-21T16:00:41.934000Z [httpworker.9] [rpc/request.cpp:241] [parse] [rpc] ThreadRPCServer method=syncwithvalidationinterfacequeue user=__cookie__
node2 2025-02-21T16:00:41.935013Z [http] [httpserver.cpp:306] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:34802
node2 2025-02-21T16:00:41.935352Z [httpworker.13] [rpc/request.cpp:24
...
💬 maflcko commented on issue "Intermittent failure in feature_fee_estimation.py in check_raw_estimates feerate = float(e["feerate"]) KeyError: 'feerate'":
(https://github.com/bitcoin/bitcoin/issues/31944#issuecomment-2678222588)
It is actually possible to reliably reproduce this:
```
./bld-cmake/test/functional/feature_fee_estimation.py --randomseed=2986529890161488286
(https://github.com/bitcoin/bitcoin/issues/31944#issuecomment-2678222588)
It is actually possible to reliably reproduce this:
```
./bld-cmake/test/functional/feature_fee_estimation.py --randomseed=2986529890161488286
💬 Eunovo commented on pull request "test: add coverage for abandoning unconfirmed transaction":
(https://github.com/bitcoin/bitcoin/pull/31943#issuecomment-2678262193)
Tested ACK https://github.com/bitcoin/bitcoin/pull/31943/commits/073a017016e62c7d52562aa61d942024379c110d
(https://github.com/bitcoin/bitcoin/pull/31943#issuecomment-2678262193)
Tested ACK https://github.com/bitcoin/bitcoin/pull/31943/commits/073a017016e62c7d52562aa61d942024379c110d
💬 Majid6561 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/3cb46c1a4ac94f4a7f25368bc2ba3c784c901b89#r152924805)
$ git show -s --format="%T" 3cb46c1a4ac94f4a7f25368bc2ba3c784c901b89
8ddfe56cfedba64667c63dd0fef6ee9584889719
$ git show -s --format="%T" 17389dc466f2acf8bfa64ce0416f3b5281445a5c
8ddfe56cfedba64667c63dd0fef6ee9584889719
(https://github.com/bitcoin/bitcoin/commit/3cb46c1a4ac94f4a7f25368bc2ba3c784c901b89#r152924805)
$ git show -s --format="%T" 3cb46c1a4ac94f4a7f25368bc2ba3c784c901b89
8ddfe56cfedba64667c63dd0fef6ee9584889719
$ git show -s --format="%T" 17389dc466f2acf8bfa64ce0416f3b5281445a5c
8ddfe56cfedba64667c63dd0fef6ee9584889719
🤔 rkrux reviewed a pull request: "descriptor: check whitespace in keys within fragments"
(https://github.com/bitcoin/bitcoin/pull/31603#pullrequestreview-2636893130)
Ok, I tested the behaviour on master with more keys and found that the RPC was successful for space containing WIF private keys, xprvs, ans xpubs, but a space containing pubkey returned an error related to invalidity as seen in the below examples.
For a space containing pubkey, in the code all the `IsHex()`, `DecodeSecret()`, `DecodeExtKey()`, and `DecodeExtPubKey()` functions fail because of the space, the 33/34 bytes size check, presence of 0, presence of 0 respectively. Thus, the `key '%s
...
(https://github.com/bitcoin/bitcoin/pull/31603#pullrequestreview-2636893130)
Ok, I tested the behaviour on master with more keys and found that the RPC was successful for space containing WIF private keys, xprvs, ans xpubs, but a space containing pubkey returned an error related to invalidity as seen in the below examples.
For a space containing pubkey, in the code all the `IsHex()`, `DecodeSecret()`, `DecodeExtKey()`, and `DecodeExtPubKey()` functions fail because of the space, the 33/34 bytes size check, presence of 0, presence of 0 respectively. Thus, the `key '%s
...
💬 rkrux commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1967529542)
> Due to Base58, pubkeys with whitespace at the beginning
end/or at the end are successfully parsed. This commit
add a parameter into `ParsePubkeyInner` to control whether
it should return an error if the first or last char of the
pubkey is a space.
Comment about the commit message: The latest code doesn't contain any parameter in `ParsePubkeyInner` that controls if the error should be returned.
Also, `pubkeys with whitespace` should be changed to `keys with whitespace` as private keys
...
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1967529542)
> Due to Base58, pubkeys with whitespace at the beginning
end/or at the end are successfully parsed. This commit
add a parameter into `ParsePubkeyInner` to control whether
it should return an error if the first or last char of the
pubkey is a space.
Comment about the commit message: The latest code doesn't contain any parameter in `ParsePubkeyInner` that controls if the error should be returned.
Also, `pubkeys with whitespace` should be changed to `keys with whitespace` as private keys
...
💬 hodlinator commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967584194)
> In any case, if this was changed, it should be applied to almost all `unwrap`,`expect`,or `panic` as well? For example, I could imagine that `expect("test failed")` could fail when the exe was missing the executable bit?
The `expect()`s here are fine since we've already run `sanity_check()` which has proper `exit(1)` behavior via `exit_help()`.
So I really do think since we are passing the user-input `filter` value, we should not be triggering an `assert!`. I think for the other commands
...
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967584194)
> In any case, if this was changed, it should be applied to almost all `unwrap`,`expect`,or `panic` as well? For example, I could imagine that `expect("test failed")` could fail when the exe was missing the executable bit?
The `expect()`s here are fine since we've already run `sanity_check()` which has proper `exit(1)` behavior via `exit_help()`.
So I really do think since we are passing the user-input `filter` value, we should not be triggering an `assert!`. I think for the other commands
...
📝 ryanofsky opened a pull request: "depends: Update libmultiprocess library to fix CI failures"
(https://github.com/bitcoin/bitcoin/pull/31945)
Bump libmultiprocess library to include bugfix https://github.com/chaincodelabs/libmultiprocess/pull/159 which should fix intermittent CI failures reported in https://github.com/bitcoin/bitcoin/issues/31921
This change is bumping the libmultiprocess version instead of cherry picking the bugfix. It could cherry-pick the bugfix instead, but there are reasons to prefer bumping the version:
- Bugfix might interact with earlier PRs, and the latest version is better tested with testing done in m
...
(https://github.com/bitcoin/bitcoin/pull/31945)
Bump libmultiprocess library to include bugfix https://github.com/chaincodelabs/libmultiprocess/pull/159 which should fix intermittent CI failures reported in https://github.com/bitcoin/bitcoin/issues/31921
This change is bumping the libmultiprocess version instead of cherry picking the bugfix. It could cherry-pick the bugfix instead, but there are reasons to prefer bumping the version:
- Bugfix might interact with earlier PRs, and the latest version is better tested with testing done in m
...