💬 darosior commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319149396)
I think Bitcoin Core should strive to limit its extensive and growing number of startup options. Optionality for the sake of it is counterproductive. On this basis, the `-datacarrier` and `-datacarriersize` options controlling a long obsolete relay policy limit are in my opinion prime candidates for removal.
That said, certain users care strongly about using those options. In these conditions, i do not see the project removing the option anytime soon. Therefore i think it's technically incorr
...
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319149396)
I think Bitcoin Core should strive to limit its extensive and growing number of startup options. Optionality for the sake of it is counterproductive. On this basis, the `-datacarrier` and `-datacarriersize` options controlling a long obsolete relay policy limit are in my opinion prime candidates for removal.
That said, certain users care strongly about using those options. In these conditions, i do not see the project removing the option anytime soon. Therefore i think it's technically incorr
...
📝 vasild opened a pull request: "net: support overriding the proxy selection in ConnectNode()"
(https://github.com/bitcoin/bitcoin/pull/33454)
Normally `ConnectNode()` could choose whether to use a proxy and which one. Make it possible to override this from the callers and same for `OpenNetworkConnection()` - pass down the proxy to `ConnectNode()`.
Document both functions.
This is useful if we want to open connections to IPv4 or IPv6 peers through the Tor SOCKS5 proxy.
---
This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](https://github.com/bitcoin/bitcoin/pull/29415). Putting i
...
(https://github.com/bitcoin/bitcoin/pull/33454)
Normally `ConnectNode()` could choose whether to use a proxy and which one. Make it possible to override this from the callers and same for `OpenNetworkConnection()` - pass down the proxy to `ConnectNode()`.
Document both functions.
This is useful if we want to open connections to IPv4 or IPv6 peers through the Tor SOCKS5 proxy.
---
This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](https://github.com/bitcoin/bitcoin/pull/29415). Putting i
...
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3319219472)
I extracted one commit from here into https://github.com/bitcoin/bitcoin/pull/33454 with the aim to reduce the size of this PR once #33454 is merged.
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3319219472)
I extracted one commit from here into https://github.com/bitcoin/bitcoin/pull/33454 with the aim to reduce the size of this PR once #33454 is merged.
💬 0xB10C commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319240689)
> > fwiw: If/when this is merged, the [draft release notes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/v30.0-Release-Notes-Draft#policy) should be updated.
>
> Well, only if it gets backported to 30.0?
Correct, I've edited my comment to reflect that.
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319240689)
> > fwiw: If/when this is merged, the [draft release notes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/v30.0-Release-Notes-Draft#policy) should be updated.
>
> Well, only if it gets backported to 30.0?
Correct, I've edited my comment to reflect that.
💬 benthecarman commented on issue "signet: disk-space-DoS due to low mining difficulty":
(https://github.com/bitcoin/bitcoin/issues/33266#issuecomment-3319342245)
@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
(https://github.com/bitcoin/bitcoin/issues/33266#issuecomment-3319342245)
@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
💬 bitschmidty commented on pull request "datacarrier: Undeprecate configuration option":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319393851)
> are you considering this as something to backport (30 et al)
That is preferred IMHO. But that is the judgment of you all and those responsible for the release process (assuming this PR gets support).
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319393851)
> are you considering this as something to backport (30 et al)
That is preferred IMHO. But that is the judgment of you all and those responsible for the release process (assuming this PR gets support).
📝 purpleKarrot opened a pull request: "CPack"
(https://github.com/bitcoin/bitcoin/pull/33455)
The goal is to build all packages (both archives and installers) from a single configuration in CMake.
### Packaging Instruction
Get a list of supported CPack generators for your platform with:
```sh
cpack --help
```
Run `cpack` from the build directory after a successful build to select multiple package generators:
```sh
cmake --build .
cpack -G '7Z;NSIS64'
```
Or build the `package` and/or `package_source` targets:
```sh
cmake --build . --target package
cmake --buil
...
(https://github.com/bitcoin/bitcoin/pull/33455)
The goal is to build all packages (both archives and installers) from a single configuration in CMake.
### Packaging Instruction
Get a list of supported CPack generators for your platform with:
```sh
cpack --help
```
Run `cpack` from the build directory after a successful build to select multiple package generators:
```sh
cmake --build .
cpack -G '7Z;NSIS64'
```
Or build the `package` and/or `package_source` targets:
```sh
cmake --build . --target package
cmake --buil
...
💬 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.