💬 0xB10C commented on issue "signet: disk-space-DoS due to low mining difficulty":
(https://github.com/bitcoin/bitcoin/issues/33266#issuecomment-3319405111)
> [@stwenhao](https://github.com/stwenhao) i don't think restricting the nonce adds any meaningful protection. The attacker can also use the merkle root as a nonce field because they do not need to actually include txs. If you limit the nonces, they can just change the merkle root until they find a block
The signet solution commits to the merkleroot. Changing it will produce invalid Signet blocks:
https://github.com/bitcoin/bitcoin/blob/953544d0286ba35dd1b07fe1921c573cba4703cf/src/signet.cpp#L
...
(https://github.com/bitcoin/bitcoin/issues/33266#issuecomment-3319405111)
> [@stwenhao](https://github.com/stwenhao) i don't think restricting the nonce adds any meaningful protection. The attacker can also use the merkle root as a nonce field because they do not need to actually include txs. If you limit the nonces, they can just change the merkle root until they find a block
The signet solution commits to the merkleroot. Changing it will produce invalid Signet blocks:
https://github.com/bitcoin/bitcoin/blob/953544d0286ba35dd1b07fe1921c573cba4703cf/src/signet.cpp#L
...
💬 janb84 commented on issue "30.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/33369#issuecomment-3319423518)
> Testing guide looks great! Some feedback below.
>
> ### 6.1 TRUC transactions
> * Perhaps make the pull request number (listed at the end of the intro paragraph) linkable like other sections?
Done
> * This guide isn't the right place for this feedback item but was hoping for guidance on whether it is expected, and if not where to report this. If I create a tx3 that is a child of tx2 (all as v3 txs) the `send` command fails silently. It failing is the correct behavior because the package siz
...
(https://github.com/bitcoin/bitcoin/issues/33369#issuecomment-3319423518)
> Testing guide looks great! Some feedback below.
>
> ### 6.1 TRUC transactions
> * Perhaps make the pull request number (listed at the end of the intro paragraph) linkable like other sections?
Done
> * This guide isn't the right place for this feedback item but was hoping for guidance on whether it is expected, and if not where to report this. If I create a tx3 that is a child of tx2 (all as v3 txs) the `send` command fails silently. It failing is the correct behavior because the package siz
...
💬 instagibbs commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319439146)
Ultimately that's a maintainer's call (burden of backporting, badgering for review, and cutting RCs), I will not put up a fuss either way.
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319439146)
Ultimately that's a maintainer's call (burden of backporting, badgering for review, and cutting RCs), I will not put up a fuss either way.
📝 viktorking7 opened a pull request: "ci(lint): Harden curl usage in 01_install.sh with fail-safe flags"
(https://github.com/bitcoin/bitcoin/pull/33456)
- **Description**:
- **What**: Use stricter curl flags when fetching ShellCheck and MLC in `ci/lint/01_install.sh`.
- **Why**: Prevent silent installs on HTTP errors and disallow non-HTTPS protocols, improving supply-chain safety.
- **Changes**:
- Replace `-sL` with `--fail --location --proto '=https' --tlsv1.2 --silent --show-error` for both downloads.
- **Impact**: CI now fails explicitly on 4xx/5xx and protocol downgrades; no behavioral changes otherwise.
(https://github.com/bitcoin/bitcoin/pull/33456)
- **Description**:
- **What**: Use stricter curl flags when fetching ShellCheck and MLC in `ci/lint/01_install.sh`.
- **Why**: Prevent silent installs on HTTP errors and disallow non-HTTPS protocols, improving supply-chain safety.
- **Changes**:
- Replace `-sL` with `--fail --location --proto '=https' --tlsv1.2 --silent --show-error` for both downloads.
- **Impact**: CI now fails explicitly on 4xx/5xx and protocol downgrades; no behavioral changes otherwise.
💬 l0rinc commented on pull request "system: improve handling around GetTotalRAM()":
(https://github.com/bitcoin/bitcoin/pull/33435#issuecomment-3319588079)
ACK 56791b582958e905e5ba5cbf172a8ea7dad1a8b0
(https://github.com/bitcoin/bitcoin/pull/33435#issuecomment-3319588079)
ACK 56791b582958e905e5ba5cbf172a8ea7dad1a8b0
💬 ajtowns commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319601018)
As it stands, this PR is based on the most recent common commit between 30.x and master, so it can be merged as-is into both branches, without any additional backporting needed.
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319601018)
As it stands, this PR is based on the most recent common commit between 30.x and master, so it can be merged as-is into both branches, without any additional backporting needed.
🤔 rkrux reviewed a pull request: "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys"
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3248963991)
Asked couple questions for clarity at 93b1d5622194165444ea9e919ccf319a5e990de0
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3248963991)
Asked couple questions for clarity at 93b1d5622194165444ea9e919ccf319a5e990de0
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2365556614)
In 821d78bddd0ecb04d36d1f860ef7e129b248ccb8 "wallet: Keep secnonces in DescriptorScriptPubKeyMan"
I believe this comment should be updated as I spent some time trying to verify it. While the scenario mentioned is a good catch which warrants a test case on its own, the following line seems required in the current tests as well, which fail if this line is commented out.
```diff
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index ff18265d70..1e2bef739a 100644
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2365556614)
In 821d78bddd0ecb04d36d1f860ef7e129b248ccb8 "wallet: Keep secnonces in DescriptorScriptPubKeyMan"
I believe this comment should be updated as I spent some time trying to verify it. While the scenario mentioned is a good catch which warrants a test case on its own, the following line seems required in the current tests as well, which fail if this line is commented out.
```diff
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index ff18265d70..1e2bef739a 100644
...
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2367767311)
In 93b1d5622194165444ea9e919ccf319a5e990de0 "test: Test MuSig2 in the wallet"
> internal key
The internal key doesn't seem to be checked for here in the above two checks. Can we remove it from the assert message?
I checked that both for `rawtr(musig(...))` and `tr(musig(..))`, the `aggregate_pubkey[2:]` is in the `scriptPubKey` in the `witness_utxo` section. I infer that its presence in only the output key is checked.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2367767311)
In 93b1d5622194165444ea9e919ccf319a5e990de0 "test: Test MuSig2 in the wallet"
> internal key
The internal key doesn't seem to be checked for here in the above two checks. Can we remove it from the assert message?
I checked that both for `rawtr(musig(...))` and `tr(musig(..))`, the `aggregate_pubkey[2:]` is in the `scriptPubKey` in the `witness_utxo` section. I infer that its presence in only the output key is checked.
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2368738533)
In https://github.com/bitcoin/bitcoin/commit/821d78bddd0ecb04d36d1f860ef7e129b248ccb8 "wallet: Keep secnonces in DescriptorScriptPubKeyMan"
> this descriptor may have a participant private key but not a musig() descriptor
I tried to add test for this scenario but the pubnonces are not added for signers that don't have the musig descriptor imported. For the following test, only the first signer that has the musig descriptor is able to add the nonce. Is there a bug or have I misunderstood th
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2368738533)
In https://github.com/bitcoin/bitcoin/commit/821d78bddd0ecb04d36d1f860ef7e129b248ccb8 "wallet: Keep secnonces in DescriptorScriptPubKeyMan"
> this descriptor may have a participant private key but not a musig() descriptor
I tried to add test for this scenario but the pubnonces are not added for signers that don't have the musig descriptor imported. For the following test, only the first signer that has the musig descriptor is able to add the nonce. Is there a bug or have I misunderstood th
...
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2368821413)
In https://github.com/bitcoin/bitcoin/commit/93b1d5622194165444ea9e919ccf319a5e990de0 "test: Test MuSig2 in the wallet"
Why does the compressed form of the `aggregate_pubkey` in the nonces and sigs section start with a `03`? Given they are valid x-only keys, shouldn't this representation have a prefix of `02` because of even Y? Is there an assumption that they would be negated later in the flow?
From the BIP:
> The plain public key must be the key found in the script and not the aggregate
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2368821413)
In https://github.com/bitcoin/bitcoin/commit/93b1d5622194165444ea9e919ccf319a5e990de0 "test: Test MuSig2 in the wallet"
Why does the compressed form of the `aggregate_pubkey` in the nonces and sigs section start with a `03`? Given they are valid x-only keys, shouldn't this representation have a prefix of `02` because of even Y? Is there an assumption that they would be negated later in the flow?
From the BIP:
> The plain public key must be the key found in the script and not the aggregate
...
✅ maflcko closed a pull request: "ci(lint): Harden curl usage in 01_install.sh with fail-safe flags"
(https://github.com/bitcoin/bitcoin/pull/33456)
(https://github.com/bitcoin/bitcoin/pull/33456)
💬 maflcko commented on pull request "ci(lint): Harden curl usage in 01_install.sh with fail-safe flags":
(https://github.com/bitcoin/bitcoin/pull/33456#issuecomment-3319631982)
Thanks, but closing for now:
* The patch is obviously wrong, as can be seen by the failing CI and the LLM linter.
* https does not add any meaningful supply-chain safety here for GitHub release downloads, so the benefit is unclear.
* If supply-chain safety was needed, it would be better to pin by a hash or commit id (and compile from source).
Adding `--fail` seems fine, but I doubt it matters much in practise.
(https://github.com/bitcoin/bitcoin/pull/33456#issuecomment-3319631982)
Thanks, but closing for now:
* The patch is obviously wrong, as can be seen by the failing CI and the LLM linter.
* https does not add any meaningful supply-chain safety here for GitHub release downloads, so the benefit is unclear.
* If supply-chain safety was needed, it would be better to pin by a hash or commit id (and compile from source).
Adding `--fail` seems fine, but I doubt it matters much in practise.
💬 jimmysong commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319644462)
While I agree with the logic for un-deprecation, I don't think this PR goes far enough as the functionality of the datacarriersize argument is fundamentally different than what it was in v29. Multiple OP_RETURN outputs are still allowed by this change, creating a situation where users cannot get the same behavior from their nodes' relay policy from previous versions through configuration.
It's still a mystery to me why multiple OP_RETURNs are being allowed since an extra OP_RETURN costs at le
...
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319644462)
While I agree with the logic for un-deprecation, I don't think this PR goes far enough as the functionality of the datacarriersize argument is fundamentally different than what it was in v29. Multiple OP_RETURN outputs are still allowed by this change, creating a situation where users cannot get the same behavior from their nodes' relay policy from previous versions through configuration.
It's still a mystery to me why multiple OP_RETURNs are being allowed since an extra OP_RETURN costs at le
...
💬 instagibbs commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319648605)
crACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
Ambivalence aside
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319648605)
crACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
Ambivalence aside
💬 instagibbs commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319665467)
> It's still a mystery to me why multiple OP_RETURNs are being allowed since an extra OP_RETURN costs at least 9 more vbytes and the same data can be stored in one OP_RETURN with some sort of (perhaps multi-byte) delimiter. Perhaps there are use-cases for multiple OP_RETURNs that can be done in one OP_RETURN that I'm not aware of?
The motivation for doing this is for situations where you cannot commit to all data efficiently otherwise. Think SIGHASH_SINGLE | ACP scenarios. The `datacarriersiz
...
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319665467)
> It's still a mystery to me why multiple OP_RETURNs are being allowed since an extra OP_RETURN costs at least 9 more vbytes and the same data can be stored in one OP_RETURN with some sort of (perhaps multi-byte) delimiter. Perhaps there are use-cases for multiple OP_RETURNs that can be done in one OP_RETURN that I'm not aware of?
The motivation for doing this is for situations where you cannot commit to all data efficiently otherwise. Think SIGHASH_SINGLE | ACP scenarios. The `datacarriersiz
...
💬 l0rinc commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2368959548)
it was specifically requested in https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2358761689
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2368959548)
it was specifically requested in https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2358761689
💬 jimmysong commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319737976)
> The motivation for doing this is for situations where you cannot commit to all data efficiently otherwise. Think SIGHASH_SINGLE | ACP scenarios. The `datacarriersize` argument applies to payloads themslves, so yes, if someone wants to do ~80 bytes of payload and can do it in one output, they should just do that.
That's fair.
Was this use case an explicit goal of the previous PR? Because I don't think users are aware of whatever potential benefits this might have. Personally, I don't cur
...
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319737976)
> The motivation for doing this is for situations where you cannot commit to all data efficiently otherwise. Think SIGHASH_SINGLE | ACP scenarios. The `datacarriersize` argument applies to payloads themslves, so yes, if someone wants to do ~80 bytes of payload and can do it in one output, they should just do that.
That's fair.
Was this use case an explicit goal of the previous PR? Because I don't think users are aware of whatever potential benefits this might have. Personally, I don't cur
...
👍 ismaelsadeeq approved a pull request: "datacarrier: Undeprecate configuration option"
(https://github.com/bitcoin/bitcoin/pull/33453#pullrequestreview-3253559489)
utACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3 🛰️
Per my review on the initial PR https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2814921270
(https://github.com/bitcoin/bitcoin/pull/33453#pullrequestreview-3253559489)
utACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3 🛰️
Per my review on the initial PR https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2814921270
📝 GregTonoski opened a pull request: "Replace text OP_RETURN with OP_SPAM for better apprehensibility and c…"
(https://github.com/bitcoin/bitcoin/pull/33457)
…hange default settings to disable spam redistribution
The patch fixes security hole (bug) in policy default settings and renames spam marker "OP_RETURN" to "OP_SPAM" for better apprehensibility.
(https://github.com/bitcoin/bitcoin/pull/33457)
…hange default settings to disable spam redistribution
The patch fixes security hole (bug) in policy default settings and renames spam marker "OP_RETURN" to "OP_SPAM" for better apprehensibility.