💬 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.
💬 Crypt-iQ commented on pull request "log: reduce excessive "rolling forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2369079142)
A new macro was not introduced in the original PR / was an intentional change so that a developer would have to use the "private" macro `LogPrintLevel_` with `should_ratelimit=false` in order to bypass rate limiting. This change is fine code-wise, but my concern is that it's now easier for a developer to accidentally introduce a logging location that an attacker can trigger. Maybe the comment for the macro can be modified to "Same as LogInfo, but never rate limited. Should be used with extreme c
...
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2369079142)
A new macro was not introduced in the original PR / was an intentional change so that a developer would have to use the "private" macro `LogPrintLevel_` with `should_ratelimit=false` in order to bypass rate limiting. This change is fine code-wise, but my concern is that it's now easier for a developer to accidentally introduce a logging location that an attacker can trigger. Maybe the comment for the macro can be modified to "Same as LogInfo, but never rate limited. Should be used with extreme c
...
💬 mzumsande commented on pull request "validation: Make ReplayBlocks interruptible":
(https://github.com/bitcoin/bitcoin/pull/30155#issuecomment-3319820076)
> Needs a rebase, but otherwise Concept ACK
Rebased for fresh CI (rebase seemed clean, didn't change anything)
(https://github.com/bitcoin/bitcoin/pull/30155#issuecomment-3319820076)
> Needs a rebase, but otherwise Concept ACK
Rebased for fresh CI (rebase seemed clean, didn't change anything)
💬 monlovesmango commented on issue "30.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/33369#issuecomment-3319820537)
I am running Arch Linux x86_64. Please let me know if there is anything I can do to help diagnose this issue.
(https://github.com/bitcoin/bitcoin/issues/33369#issuecomment-3319820537)
I am running Arch Linux x86_64. Please let me know if there is anything I can do to help diagnose this issue.
💬 andrewtoth commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3319833946)
Need to update https://github.com/bitcoin/bitcoin/pull/33454/files#diff-00021eed586a482abdb09d6cdada1d90115abe988a91421851960e26658bed02L2882 `strDest` -> `pszDest` to placate tidy.
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3319833946)
Need to update https://github.com/bitcoin/bitcoin/pull/33454/files#diff-00021eed586a482abdb09d6cdada1d90115abe988a91421851960e26658bed02L2882 `strDest` -> `pszDest` to placate tidy.
🚀 hebasto merged a pull request: "system: improve handling around GetTotalRAM()"
(https://github.com/bitcoin/bitcoin/pull/33435)
(https://github.com/bitcoin/bitcoin/pull/33435)
💬 ryanofsky commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2369187858)
re: https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2365640546
> This test only calls `submitSolution()` once, unless I'm missing something?
Sorry I just misread the diff, didn't notice the first call was being deleted
(https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2369187858)
re: https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2365640546
> This test only calls `submitSolution()` once, unless I'm missing something?
Sorry I just misread the diff, didn't notice the first call was being deleted
💬 monlovesmango commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3319935660)
I was doing some extra adhoc testing for section 7.1 in the testing guide and hoping for guidance on whether the result is expected or not. 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 size shouldn't be greater than 2, but was unsure if the fail should be silent? Below will create the 3 truc transactions and check if tx3 is in mempool.
```bash
TRUC_address=$(bcli30 getnewaddress)
tx=$(bcli30 -n
...
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3319935660)
I was doing some extra adhoc testing for section 7.1 in the testing guide and hoping for guidance on whether the result is expected or not. 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 size shouldn't be greater than 2, but was unsure if the fail should be silent? Below will create the 3 truc transactions and check if tx3 is in mempool.
```bash
TRUC_address=$(bcli30 getnewaddress)
tx=$(bcli30 -n
...
💬 hebasto commented on pull request "depends: Switch from multilib to platform-specific toolchains":
(https://github.com/bitcoin/bitcoin/pull/32162#issuecomment-3319941069)
My Guix build:
```
aarch64
01ad8136b264db6408b4eaec324a8950aa1ba5096f0bad9e2bd5dcb21cf4ef30 guix-build-3e69b6973607/output/aarch64-linux-gnu/SHA256SUMS.part
7a6abbd1721b008f05358acee75662bc76028cb65da8bbc2435ccc258429b064 guix-build-3e69b6973607/output/aarch64-linux-gnu/bitcoin-3e69b6973607-aarch64-linux-gnu-debug.tar.gz
a88021c9867d1859784195354d2a79d400b0b24be9baa06968236dffccf0bb71 guix-build-3e69b6973607/output/aarch64-linux-gnu/bitcoin-3e69b6973607-aarch64-linux-gnu.tar.gz
0ece5435
...
(https://github.com/bitcoin/bitcoin/pull/32162#issuecomment-3319941069)
My Guix build:
```
aarch64
01ad8136b264db6408b4eaec324a8950aa1ba5096f0bad9e2bd5dcb21cf4ef30 guix-build-3e69b6973607/output/aarch64-linux-gnu/SHA256SUMS.part
7a6abbd1721b008f05358acee75662bc76028cb65da8bbc2435ccc258429b064 guix-build-3e69b6973607/output/aarch64-linux-gnu/bitcoin-3e69b6973607-aarch64-linux-gnu-debug.tar.gz
a88021c9867d1859784195354d2a79d400b0b24be9baa06968236dffccf0bb71 guix-build-3e69b6973607/output/aarch64-linux-gnu/bitcoin-3e69b6973607-aarch64-linux-gnu.tar.gz
0ece5435
...
💬 Kruwed commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319944425)
>**Many current Bitcoin Core users want to continue using this option**
This statement is based on public postings from many Bitcoin Core users and not a formal survey.
Popularity is not a sufficient reason alone, can you provide any of the underlying reasons why these users want to continue using this option?
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3319944425)
>**Many current Bitcoin Core users want to continue using this option**
This statement is based on public postings from many Bitcoin Core users and not a formal survey.
Popularity is not a sufficient reason alone, can you provide any of the underlying reasons why these users want to continue using this option?
💬 waketraindev commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320038208)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320038208)
Concept ACK
💬 adam3us commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320053572)
> 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
...
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320053572)
> 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
...
👍 jonatack approved a pull request: "docs: Undeprecate datacarrier and datacarriersize configuration options"
(https://github.com/bitcoin/bitcoin/pull/33453#pullrequestreview-3253916968)
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
Agree with the change, reviewed, verified that it is clean revert of 0b4048c73385166144d0b, built/ran tests locally.
(https://github.com/bitcoin/bitcoin/pull/33453#pullrequestreview-3253916968)
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
Agree with the change, reviewed, verified that it is clean revert of 0b4048c73385166144d0b, built/ran tests locally.
💬 jonatack commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320067548)
Note: the translation files would need updating?
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320067548)
Note: the translation files would need updating?