π fanquake merged a pull request: "[29.x] test: Do not pass tests on unhandled exceptions"
(https://github.com/bitcoin/bitcoin/pull/33046)
(https://github.com/bitcoin/bitcoin/pull/33046)
π¬ bigshiny90 commented on pull request "test: Add functional tests for blockreconstructionextratxn parameter":
(https://github.com/bitcoin/bitcoin/pull/33023#issuecomment-3113073248)
updated to fix CI Lint fail (whitespace removal)
(https://github.com/bitcoin/bitcoin/pull/33023#issuecomment-3113073248)
updated to fix CI Lint fail (whitespace removal)
π fanquake merged a pull request: "test: check proper OP_2ROT behavior"
(https://github.com/bitcoin/bitcoin/pull/33047)
(https://github.com/bitcoin/bitcoin/pull/33047)
π¬ bigshiny90 commented on pull request "test: Add functional tests for blockreconstructionextratxn parameter":
(https://github.com/bitcoin/bitcoin/pull/33023#issuecomment-3113104256)
updated to fix CI Lint errors
(https://github.com/bitcoin/bitcoin/pull/33023#issuecomment-3113104256)
updated to fix CI Lint errors
π¬ laanwj commented on pull request "Enable `-natpmp` by default":
(https://github.com/bitcoin/bitcoin/pull/33004#issuecomment-3113109911)
Post-merge ACK b2d07f872c58af9cfdf9f9a4af0645376f9b98cb
(https://github.com/bitcoin/bitcoin/pull/33004#issuecomment-3113109911)
Post-merge ACK b2d07f872c58af9cfdf9f9a4af0645376f9b98cb
π€ fanquake reviewed a pull request: "ci: Use optimized Debug build type in test-each-commit"
(https://github.com/bitcoin/bitcoin/pull/32888#pullrequestreview-3051259900)
Seems fine to add this and be using `mold` in one job.
> This ci run took 30min7sec (https://github.com/bitcoin/bitcoin/actions/runs/16112781421/job/45459901801?pr=32888)
This doesn't actually show the new changes here right?
(https://github.com/bitcoin/bitcoin/pull/32888#pullrequestreview-3051259900)
Seems fine to add this and be using `mold` in one job.
> This ci run took 30min7sec (https://github.com/bitcoin/bitcoin/actions/runs/16112781421/job/45459901801?pr=32888)
This doesn't actually show the new changes here right?
π¬ fanquake commented on pull request "ci: Use optimized Debug build type in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/32888#discussion_r2228252688)
The CMake here is one version too old, otherwise we could also have used `CMAKE_LINKER_TYPE=MOLD`
(https://github.com/bitcoin/bitcoin/pull/32888#discussion_r2228252688)
The CMake here is one version too old, otherwise we could also have used `CMAKE_LINKER_TYPE=MOLD`
π fanquake merged a pull request: "ci: Use optimized Debug build type in test-each-commit"
(https://github.com/bitcoin/bitcoin/pull/32888)
(https://github.com/bitcoin/bitcoin/pull/32888)
π fanquake's pull request is ready for review: "test: Add functional tests for blockreconstructionextratxn parameter"
(https://github.com/bitcoin/bitcoin/pull/33023)
(https://github.com/bitcoin/bitcoin/pull/33023)
π¬ stickies-v commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#issuecomment-3113155030)
Force-pushed from [319abd6](https://github.com/bitcoin/bitcoin/commit/319abd609b3d2194e7a05454b6af55b6f16adbf1) to [b8772ef](https://github.com/bitcoin/bitcoin/commit/b8772efebace3f4f0cfed6479648bce7a521ff27) to remove a `std::string` copy in `rpc/mining.cpp`, addressing (part of) @pablomartin4btc's feedback.
---
> It seems like weβre going from `string` β `string_view` β `string`
`string_view` is a non-owning read-only reference, so by definition it has to point to some other string-li
...
(https://github.com/bitcoin/bitcoin/pull/32983#issuecomment-3113155030)
Force-pushed from [319abd6](https://github.com/bitcoin/bitcoin/commit/319abd609b3d2194e7a05454b6af55b6f16adbf1) to [b8772ef](https://github.com/bitcoin/bitcoin/commit/b8772efebace3f4f0cfed6479648bce7a521ff27) to remove a `std::string` copy in `rpc/mining.cpp`, addressing (part of) @pablomartin4btc's feedback.
---
> It seems like weβre going from `string` β `string_view` β `string`
`string_view` is a non-owning read-only reference, so by definition it has to point to some other string-li
...
π Sjors opened a pull request: "Don't fix Python patch version"
(https://github.com/bitcoin/bitcoin/pull/33051)
Fixing the patch version can cause problems when interacting with other programs such as HWI.
E.g. if the user installed Python 3.10.14 and 3.10.16 through PyEnv, if HWI was locked to 3.10 it would default to 3.10.16. HWI then can't find any of its dependencies.
See also https://github.com/bitcoin-core/HWI/pull/695
(https://github.com/bitcoin/bitcoin/pull/33051)
Fixing the patch version can cause problems when interacting with other programs such as HWI.
E.g. if the user installed Python 3.10.14 and 3.10.16 through PyEnv, if HWI was locked to 3.10 it would default to 3.10.16. HWI then can't find any of its dependencies.
See also https://github.com/bitcoin-core/HWI/pull/695
π¬ maflcko commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3113282579)
the lint CI failed
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3113282579)
the lint CI failed
π¬ josibake commented on pull request "Silent Payments: sending":
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r2228447040)
This is fine, actually. What we are doing here is making sure to _never_ include a taproot with script path spend or a witness unknown spend. We can include Pay2Pubkey inputs, so long as there is at least one silent payments eligible input.
In the `CreateSilentPaymentsOutputs` function, we check for at least one eligible input here: https://github.com/bitcoin/bitcoin/blob/4225ebc65595b5ef0f661b4f41e3e21266b93575/src/wallet/spend.cpp#L1116, by calling the `IsInputForSharedSecretDerivation` fun
...
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r2228447040)
This is fine, actually. What we are doing here is making sure to _never_ include a taproot with script path spend or a witness unknown spend. We can include Pay2Pubkey inputs, so long as there is at least one silent payments eligible input.
In the `CreateSilentPaymentsOutputs` function, we check for at least one eligible input here: https://github.com/bitcoin/bitcoin/blob/4225ebc65595b5ef0f661b4f41e3e21266b93575/src/wallet/spend.cpp#L1116, by calling the `IsInputForSharedSecretDerivation` fun
...
π¬ b-l-u-e commented on pull request "rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures":
(https://github.com/bitcoin/bitcoin/pull/33014#issuecomment-3113382620)
@achow101...You were right, my initial approach was incorrect. I now understand that CHECK_NONFATAL is correctly protecting the invariant that a PSBT marked as complete must be finalizable.
The actual bug, as you pointed out, was that the completeness check was too lenient, only checking for the presence of a signature rather than its cryptographic validity.
After ensuring that the logic first verifies the existing signatures before declaring the PSBT complete (the approach you suggested w
...
(https://github.com/bitcoin/bitcoin/pull/33014#issuecomment-3113382620)
@achow101...You were right, my initial approach was incorrect. I now understand that CHECK_NONFATAL is correctly protecting the invariant that a PSBT marked as complete must be finalizable.
The actual bug, as you pointed out, was that the completeness check was too lenient, only checking for the presence of a signature rather than its cryptographic validity.
After ensuring that the logic first verifies the existing signatures before declaring the PSBT complete (the approach you suggested w
...
π€ darosior reviewed a pull request: "net, validation: don't punish peers for consensus-invalid txs"
(https://github.com/bitcoin/bitcoin/pull/33050#pullrequestreview-3051560671)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/33050#pullrequestreview-3051560671)
Concept ACK.
π¬ darosior commented on pull request "net, validation: don't punish peers for consensus-invalid txs":
(https://github.com/bitcoin/bitcoin/pull/33050#discussion_r2228448364)
You might as well drop `expect_disconnect` in the templates then, instead of setting it to False both there and here? Also the reconnect logic just below here is now dead code.
(https://github.com/bitcoin/bitcoin/pull/33050#discussion_r2228448364)
You might as well drop `expect_disconnect` in the templates then, instead of setting it to False both there and here? Also the reconnect logic just below here is now dead code.
π¬ darosior commented on pull request "net, validation: don't punish peers for consensus-invalid txs":
(https://github.com/bitcoin/bitcoin/pull/33050#discussion_r2228460217)
This could make a non-standard transaction error with "non-mandatory-script-verify-flag", which would be confusing. I think we can just drop the mandatoriness language.
```diff
- return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(result->first)), result->second);
+ return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("script validation failed (%s)", ScriptErrorString(result->first)), result->second);
...
(https://github.com/bitcoin/bitcoin/pull/33050#discussion_r2228460217)
This could make a non-standard transaction error with "non-mandatory-script-verify-flag", which would be confusing. I think we can just drop the mandatoriness language.
```diff
- return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(result->first)), result->second);
+ return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("script validation failed (%s)", ScriptErrorString(result->first)), result->second);
...
π¬ darosior commented on pull request "script: return verification flag responsible for error upon validation failure":
(https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3113410847)
> Someone recently made the point that we already have a lot of code to maintain in bitcoin core
A wise man! More seriously, i was merely pointing out the tradeoff here. Not saying this PR is the better alternative to #33050. I am out of rationalization for the existing behaviour anyways, so might as well take a red diff over a green one.
(https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3113410847)
> Someone recently made the point that we already have a lot of code to maintain in bitcoin core
A wise man! More seriously, i was merely pointing out the tradeoff here. Not saying this PR is the better alternative to #33050. I am out of rationalization for the existing behaviour anyways, so might as well take a red diff over a green one.
π¬ marcofleon commented on pull request "[29.x] Backport #32521":
(https://github.com/bitcoin/bitcoin/pull/33013#issuecomment-3113414394)
reACK f25dc84b2892e6bdbbd0471add9fcb2757700981
(https://github.com/bitcoin/bitcoin/pull/33013#issuecomment-3113414394)
reACK f25dc84b2892e6bdbbd0471add9fcb2757700981
π¬ Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3113414578)
Rebased after #32888. I ended up indenting the list of apt installed packages for the `test-each-commit` job, which might reduce future conflicts.
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3113414578)
Rebased after #32888. I ended up indenting the list of apt installed packages for the `test-each-commit` job, which might reduce future conflicts.