💬 instagibbs commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2180105702)
I think that's still ambiguous as it does read like a taproot sigops budget sense where the opcode itself must be executed to count
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2180105702)
I think that's still ambiguous as it does read like a taproot sigops budget sense where the opcode itself must be executed to count
💬 luke-jr commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2180106942)
This is still wrong. If you bury a sigop in (false) OP_IF, they are still counted even though not actually executed
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2180106942)
This is still wrong. If you bury a sigop in (false) OP_IF, they are still counted even though not actually executed
💬 fanquake commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-3027954533)
Added this to `30.0`.
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-3027954533)
Added this to `30.0`.
👍 brunoerg approved a pull request: "fuzz: Make process_message(s) more deterministic"
(https://github.com/bitcoin/bitcoin/pull/32822#pullrequestreview-2979255643)
light code review ACK fac673d434b4d32d8c44dcc50a3655ba4a1816de
(https://github.com/bitcoin/bitcoin/pull/32822#pullrequestreview-2979255643)
light code review ACK fac673d434b4d32d8c44dcc50a3655ba4a1816de
✅ fanquake closed a pull request: "doc: Add hint about avoiding spaces in paths when building on Windows"
(https://github.com/bitcoin/bitcoin/pull/32397)
(https://github.com/bitcoin/bitcoin/pull/32397)
💬 fanquake commented on pull request "doc: Add hint about avoiding spaces in paths when building on Windows":
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-3027979421)
Closing for now. @hebasto maybe you can followup with your own suggestion.
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-3027979421)
Closing for now. @hebasto maybe you can followup with your own suggestion.
💬 bigshiny90 commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3027980445)
Changes are in my branch: https://github.com/bigshiny90/bitcoin/tree/fix-pr-32844-witness
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3027980445)
Changes are in my branch: https://github.com/bigshiny90/bitcoin/tree/fix-pr-32844-witness
💬 fanquake commented on pull request "allocators: Apply manual ASan poisoning to `PoolResource`":
(https://github.com/bitcoin/bitcoin/pull/32581#issuecomment-3027988340)
Added this to 30.0.
cc @marcofleon
(https://github.com/bitcoin/bitcoin/pull/32581#issuecomment-3027988340)
Added this to 30.0.
cc @marcofleon
🚀 fanquake merged a pull request: "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)"
(https://github.com/bitcoin/bitcoin/pull/32564)
(https://github.com/bitcoin/bitcoin/pull/32564)
💬 fanquake commented on pull request "util: C++20 `ToIntegral()` Improvement":
(https://github.com/bitcoin/bitcoin/pull/32522#discussion_r2180168906)
> Otherwise, it will become harder to diagnose bugs in the code with the compiler or with sanitizers.
@dergoegge any opinion?
(https://github.com/bitcoin/bitcoin/pull/32522#discussion_r2180168906)
> Otherwise, it will become harder to diagnose bugs in the code with the compiler or with sanitizers.
@dergoegge any opinion?
📝 Sjors opened a pull request: "wallet: allow skipping script paths"
(https://github.com/bitcoin/bitcoin/pull/32857)
While testing MuSig2 integration in #29675 I ran into a Ledger bug https://github.com/LedgerHQ/app-bitcoin-new/issues/329 that was triggered because we signed for a script path in addition to the key path. The bug itself will be fixed, but it seems useful in general to be able to skip script path signing.
For example, you may have a 2-of-2 musig() descriptor with some sort of fallback after coins are N blocks old. For both on chain privacy and fee reasons, you don't want to use this path unle
...
(https://github.com/bitcoin/bitcoin/pull/32857)
While testing MuSig2 integration in #29675 I ran into a Ledger bug https://github.com/LedgerHQ/app-bitcoin-new/issues/329 that was triggered because we signed for a script path in addition to the key path. The bug itself will be fixed, but it seems useful in general to be able to skip script path signing.
For example, you may have a 2-of-2 musig() descriptor with some sort of fallback after coins are N blocks old. For both on chain privacy and fee reasons, you don't want to use this path unle
...
💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3028050886)
Meanwhile https://github.com/LedgerHQ/app-bitcoin-new/issues/329 has been figured out and I was able to do a keypath spend on the more complicated policy. That's a good sign for interoperability.
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3028050886)
Meanwhile https://github.com/LedgerHQ/app-bitcoin-new/issues/329 has been figured out and I was able to do a keypath spend on the more complicated policy. That's a good sign for interoperability.
💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2180187726)
Given that the coinbase maturity tests also use invalidateblock, let's just keep this particular test untouched for now. This test setup is going to have to be a bit more advanced and can be done in a follow-up
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2180187726)
Given that the coinbase maturity tests also use invalidateblock, let's just keep this particular test untouched for now. This test setup is going to have to be a bit more advanced and can be done in a follow-up
💬 fanquake commented on pull request "cmake: Improve Python robustness and test usability":
(https://github.com/bitcoin/bitcoin/pull/31233#issuecomment-3028091142)
Guix Build:
```bash
cc67459424a62c706895f382cff677c1e0c0ecd7ee7f43429d64d1f5d872f151 guix-build-67dc7523f3e1/output/aarch64-linux-gnu/SHA256SUMS.part
3c1eb6f629cc3b00d7e3803fc33383aea1f7cf6646d4de3e5c96d30923a1a795 guix-build-67dc7523f3e1/output/aarch64-linux-gnu/bitcoin-67dc7523f3e1-aarch64-linux-gnu-debug.tar.gz
494c55c6843e0d1f8aa133ee4c7685f06f04ca4c3dcb8ccee02aa11686a64350 guix-build-67dc7523f3e1/output/aarch64-linux-gnu/bitcoin-67dc7523f3e1-aarch64-linux-gnu.tar.gz
e12c0167012af21d
...
(https://github.com/bitcoin/bitcoin/pull/31233#issuecomment-3028091142)
Guix Build:
```bash
cc67459424a62c706895f382cff677c1e0c0ecd7ee7f43429d64d1f5d872f151 guix-build-67dc7523f3e1/output/aarch64-linux-gnu/SHA256SUMS.part
3c1eb6f629cc3b00d7e3803fc33383aea1f7cf6646d4de3e5c96d30923a1a795 guix-build-67dc7523f3e1/output/aarch64-linux-gnu/bitcoin-67dc7523f3e1-aarch64-linux-gnu-debug.tar.gz
494c55c6843e0d1f8aa133ee4c7685f06f04ca4c3dcb8ccee02aa11686a64350 guix-build-67dc7523f3e1/output/aarch64-linux-gnu/bitcoin-67dc7523f3e1-aarch64-linux-gnu.tar.gz
e12c0167012af21d
...
💬 maflcko commented on pull request "Refactor: Redefine CTransaction equality to include witness data":
(https://github.com/bitcoin/bitcoin/pull/32723#issuecomment-3028112892)
Just for reference and fun, not as a pull request, the `EqualsOptions` patch would look something like:
```diff
diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
index ad81727094..7eb8ded479 100644
--- a/src/primitives/transaction.h
+++ b/src/primitives/transaction.h
@@ -289,6 +289,10 @@ inline CAmount CalculateOutputValue(const TxType& tx)
}
+struct EqualsOptions {
+ bool include_script_sig{true};
+ bool include_witness_data{true};
+};
/** The
...
(https://github.com/bitcoin/bitcoin/pull/32723#issuecomment-3028112892)
Just for reference and fun, not as a pull request, the `EqualsOptions` patch would look something like:
```diff
diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
index ad81727094..7eb8ded479 100644
--- a/src/primitives/transaction.h
+++ b/src/primitives/transaction.h
@@ -289,6 +289,10 @@ inline CAmount CalculateOutputValue(const TxType& tx)
}
+struct EqualsOptions {
+ bool include_script_sig{true};
+ bool include_witness_data{true};
+};
/** The
...
💬 maflcko commented on pull request "feature_taproot: sample tx version border values more":
(https://github.com/bitcoin/bitcoin/pull/32841#discussion_r2180240391)
i guess it is not enough and the truc rules need to be checked as well. Otherwise:
```
[10:00:08.436] �[0;36m test_framework.authproxy.JSONRPCException: TRUC-violation, version=3 tx 18cf284e19b7df3927bb95ea6a15d7d710a180bb62ccf18ad05948915c85171f (wtxid=3a4704c5da033ee18e46813915c5bad5747277e40f9684689bb66ed91ac85256) is too big: 25250 > 10000 virtual bytes (-26)�[0m
```
https://cirrus-ci.com/task/6341200170450944?logs=ci#L2708
(https://github.com/bitcoin/bitcoin/pull/32841#discussion_r2180240391)
i guess it is not enough and the truc rules need to be checked as well. Otherwise:
```
[10:00:08.436] �[0;36m test_framework.authproxy.JSONRPCException: TRUC-violation, version=3 tx 18cf284e19b7df3927bb95ea6a15d7d710a180bb62ccf18ad05948915c85171f (wtxid=3a4704c5da033ee18e46813915c5bad5747277e40f9684689bb66ed91ac85256) is too big: 25250 > 10000 virtual bytes (-26)�[0m
```
https://cirrus-ci.com/task/6341200170450944?logs=ci#L2708
🤔 rkrux reviewed a pull request: "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet"
(https://github.com/bitcoin/bitcoin/pull/31423#pullrequestreview-2979454191)
LGTM ACK b78990734621b8fe46c68a6e7edaf1fbd2f7d351
I verified from `MigrateToDescriptor` that `data.desc_spkms` can contain only spendable SPKMs as the watch-only ones are filtered out in `data.watch_descs` when the descriptor can't be converted to a private string. Rest two cases of HDChains and NonHD-Keys push SPKMs in `data.desc_spkms` and they both have access to the corresponding key material. So, I believe the following condition seems sufficient to check for spendable material.
```cp
...
(https://github.com/bitcoin/bitcoin/pull/31423#pullrequestreview-2979454191)
LGTM ACK b78990734621b8fe46c68a6e7edaf1fbd2f7d351
I verified from `MigrateToDescriptor` that `data.desc_spkms` can contain only spendable SPKMs as the watch-only ones are filtered out in `data.watch_descs` when the descriptor can't be converted to a private string. Rest two cases of HDChains and NonHD-Keys push SPKMs in `data.desc_spkms` and they both have access to the corresponding key material. So, I believe the following condition seems sufficient to check for spendable material.
```cp
...
💬 instagibbs commented on pull request "feature_taproot: sample tx version border values more":
(https://github.com/bitcoin/bitcoin/pull/32841#discussion_r2180249602)
There are no in-mempool deps afaik, so checking the vsize of `tx` should suffice?
(https://github.com/bitcoin/bitcoin/pull/32841#discussion_r2180249602)
There are no in-mempool deps afaik, so checking the vsize of `tx` should suffice?
👍 theStack approved a pull request: "test: fix an incorrect `feature_fee_estimation.py` subtest"
(https://github.com/bitcoin/bitcoin/pull/32463#pullrequestreview-2979460112)
Light ACK 9b75cfda4d62a0a3bde402503244dd57e1621a12
I'm not familiar with the inner workings of our fee estimation code, but only considering how the "feerate" result is constructed in the `estimatesmartfee` RPC, the changes look good to me. Increasing the max block weight params by 4000 to compensate for https://github.com/bitcoin/bitcoin/pull/31384 also makes sense.
> Nice work finding the issue. For some reason, I couldn't reproduce using `--random=3450808900320758527`. But `--random=413
...
(https://github.com/bitcoin/bitcoin/pull/32463#pullrequestreview-2979460112)
Light ACK 9b75cfda4d62a0a3bde402503244dd57e1621a12
I'm not familiar with the inner workings of our fee estimation code, but only considering how the "feerate" result is constructed in the `estimatesmartfee` RPC, the changes look good to me. Increasing the max block weight params by 4000 to compensate for https://github.com/bitcoin/bitcoin/pull/31384 also makes sense.
> Nice work finding the issue. For some reason, I couldn't reproduce using `--random=3450808900320758527`. But `--random=413
...
📝 hebasto opened a pull request: "doc: Add workaround for vcpkg issue with paths with embedded spaces"
(https://github.com/bitcoin/bitcoin/pull/32858)
This PR is an alternative to https://github.com/bitcoin/bitcoin/pull/32397 suggested in https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2883438040.
(https://github.com/bitcoin/bitcoin/pull/32858)
This PR is an alternative to https://github.com/bitcoin/bitcoin/pull/32397 suggested in https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2883438040.