💬 ajtowns commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2086377216)
This is only a limit on the arbitrary data you're encoding in OP_RETURN scriptPubKeys, not a limit on the overall transaction size.
If you want to encode additional data, there are three approaches: extend an existing PUSH with extra data, add an additional PUSH, or add an additional output. These have an overhead of roughly 0, 1 and 2 with respect to this limit, and an overhead of 0, 1 and 10 vbytes wrt transaction weight with a corresponding impact on fees needed. If you did indeed have 83
...
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2086377216)
This is only a limit on the arbitrary data you're encoding in OP_RETURN scriptPubKeys, not a limit on the overall transaction size.
If you want to encode additional data, there are three approaches: extend an existing PUSH with extra data, add an additional PUSH, or add an additional output. These have an overhead of roughly 0, 1 and 2 with respect to this limit, and an overhead of 0, 1 and 10 vbytes wrt transaction weight with a corresponding impact on fees needed. If you did indeed have 83
...
📝 maflcko opened a pull request: "lint: Check for missing trailing newline"
(https://github.com/bitcoin/bitcoin/pull/32477)
A missing trailing newline is harmless, but a bit problematic:
* `git` shows a warning by default
* After another line is appended, the diff will be verbose and `git blame` will be wrong for the "untouched" line.
Fix the problems by just requiring what is already the default, see also https://github.com/bitcoin/bitcoin/blob/663a9cabf811e2fc53102bc6da00d09fc99d1d81/.editorconfig#L9 and https://github.com/bitcoin/bitcoin/blob/663a9cabf811e2fc53102bc6da00d09fc99d1d81/test/lint/test_runner/sr
...
(https://github.com/bitcoin/bitcoin/pull/32477)
A missing trailing newline is harmless, but a bit problematic:
* `git` shows a warning by default
* After another line is appended, the diff will be verbose and `git blame` will be wrong for the "untouched" line.
Fix the problems by just requiring what is already the default, see also https://github.com/bitcoin/bitcoin/blob/663a9cabf811e2fc53102bc6da00d09fc99d1d81/.editorconfig#L9 and https://github.com/bitcoin/bitcoin/blob/663a9cabf811e2fc53102bc6da00d09fc99d1d81/test/lint/test_runner/sr
...
💬 vasild commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2086457036)
Yes, that is the same in `master` so no change in behavior by this PR. In master it is using a magic number `9050` whereas here it uses the constant `DEFAULT_TOR_SOCKS_PORT`.
`-proxy=127.0.0.1=cjdns` would be interpreted as `-proxy=127.0.0.1:9050=cjdns` which might be the correct config, or if not then the user could use `-proxy=127.0.0.1:1234=cjdns` to specify another port.
Personally, I would favor a more dumb implementation that does not do such automations and guessings on behalf of th
...
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2086457036)
Yes, that is the same in `master` so no change in behavior by this PR. In master it is using a magic number `9050` whereas here it uses the constant `DEFAULT_TOR_SOCKS_PORT`.
`-proxy=127.0.0.1=cjdns` would be interpreted as `-proxy=127.0.0.1:9050=cjdns` which might be the correct config, or if not then the user could use `-proxy=127.0.0.1:1234=cjdns` to specify another port.
Personally, I would favor a more dumb implementation that does not do such automations and guessings on behalf of th
...
💬 vasild commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2086457264)
Done.
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2086457264)
Done.
💬 vasild commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2086458031)
Done.
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2086458031)
Done.
💬 vasild commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#issuecomment-2875900750)
`2059eaa692...e98c51fcce`: address suggestions
(https://github.com/bitcoin/bitcoin/pull/32425#issuecomment-2875900750)
`2059eaa692...e98c51fcce`: address suggestions
💬 hebasto commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2875926603)
Rebased due to a conflict with the merged bitcoin/bitcoin#32458.
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2875926603)
Rebased due to a conflict with the merged bitcoin/bitcoin#32458.
💬 hebasto commented on pull request "Update `secp256k1` subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2875949757)
Rebased and updated.
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2875949757)
Rebased and updated.
💬 maflcko commented on pull request "lint: Check for missing trailing newline":
(https://github.com/bitcoin/bitcoin/pull/32477#issuecomment-2875950885)
This should also catch issues where existing newlines are removed (and make the diff even more verbose): https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072592239 (cc @l0rinc)
(https://github.com/bitcoin/bitcoin/pull/32477#issuecomment-2875950885)
This should also catch issues where existing newlines are removed (and make the diff even more verbose): https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072592239 (cc @l0rinc)
⚠️ 1440000bytes opened an issue: "Allow multiple data outputs in `createrawtransaction` and `createpsbt`"
(https://github.com/bitcoin/bitcoin/issues/32478)
### Motivation
> Each key may only appear once, i.e. there can only be one 'data' output, and no address may be duplicated.
https://bitcoincore.org/en/doc/29.0.0/rpc/rawtransactions/createrawtransaction/
If and when https://github.com/bitcoin/bitcoin/pull/32406 gets merged, it will be helpful to use multiple data outputs while creating a transaction.
Related comment: https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2875133307
### Possible solution
Remove the check for duplicate d
...
(https://github.com/bitcoin/bitcoin/issues/32478)
### Motivation
> Each key may only appear once, i.e. there can only be one 'data' output, and no address may be duplicated.
https://bitcoincore.org/en/doc/29.0.0/rpc/rawtransactions/createrawtransaction/
If and when https://github.com/bitcoin/bitcoin/pull/32406 gets merged, it will be helpful to use multiple data outputs while creating a transaction.
Related comment: https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2875133307
### Possible solution
Remove the check for duplicate d
...
💬 maflcko commented on issue "Allow multiple data outputs in `createrawtransaction` and `createpsbt`":
(https://github.com/bitcoin/bitcoin/issues/32478#issuecomment-2876030352)
> If and when [#32406](https://github.com/bitcoin/bitcoin/pull/32406) gets merged
The policy change isn't merged, and if it was merged, any wallet or rpc features usually come one release later. So I've removed the "good first issue" label and moved the milestone back.
(https://github.com/bitcoin/bitcoin/issues/32478#issuecomment-2876030352)
> If and when [#32406](https://github.com/bitcoin/bitcoin/pull/32406) gets merged
The policy change isn't merged, and if it was merged, any wallet or rpc features usually come one release later. So I've removed the "good first issue" label and moved the milestone back.
💬 fanquake commented on pull request "[POC] build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-2876119294)
```bash
test_bitcoin.vcxproj -> D:\a\bitcoin\bitcoin\build\bin\Release\test_bitcoin.exe
lld-link : warning : found both wWinMain and WinMain; using latter [D:\a\bitcoin\bitcoin\build\src\qt\bitcoin-qt.vcxproj]
lld-link : error : undefined symbol: __declspec(dllimport) PathRemoveFileSpecW [D:\a\bitcoin\bitcoin\build\src\qt\bitcoin-qt.vcxproj]
>>> referenced by bitcoinqt.lib(guiutil.obj):(bool __cdecl GUIUtil::SetStartOnSystemStartup(bool))
lld-link : error : undefined symbol: __declspec
...
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-2876119294)
```bash
test_bitcoin.vcxproj -> D:\a\bitcoin\bitcoin\build\bin\Release\test_bitcoin.exe
lld-link : warning : found both wWinMain and WinMain; using latter [D:\a\bitcoin\bitcoin\build\src\qt\bitcoin-qt.vcxproj]
lld-link : error : undefined symbol: __declspec(dllimport) PathRemoveFileSpecW [D:\a\bitcoin\bitcoin\build\src\qt\bitcoin-qt.vcxproj]
>>> referenced by bitcoinqt.lib(guiutil.obj):(bool __cdecl GUIUtil::SetStartOnSystemStartup(bool))
lld-link : error : undefined symbol: __declspec
...
👍 stickies-v approved a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2836196063)
ACK c164064c266c518588d9d9175f9b14140ee751b6. I've left a few nits, but nothing blocking, especially in a PR with this much activity.
With the new default, I think users disabling or lowering `datacarriersize` are mostly reducing the performance of their own node, so I am in support of removing it completely. However, some people seem really keen on continuing to be able to do so, so pragmatically I think the approach taken in this PR is sensible. Changing the default is what's really importa
...
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2836196063)
ACK c164064c266c518588d9d9175f9b14140ee751b6. I've left a few nits, but nothing blocking, especially in a PR with this much activity.
With the new default, I think users disabling or lowering `datacarriersize` are mostly reducing the performance of their own node, so I am in support of removing it completely. However, some people seem really keen on continuing to be able to do so, so pragmatically I think the approach taken in this PR is sensible. Changing the default is what's really importa
...
💬 stickies-v commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2086524579)
nit: would prefer avoiding the literal by keeping `MAX_OP_RETURN_RELAY`, or at least document why you picked 99800.
<details>
<summary>git diff on c164064c26</summary>
```diff
diff --git a/test/functional/mempool_datacarrier.py b/test/functional/mempool_datacarrier.py
index 6347215b86..8eab4ce6b2 100755
--- a/test/functional/mempool_datacarrier.py
+++ b/test/functional/mempool_datacarrier.py
@@ -5,6 +5,7 @@
"""Test datacarrier functionality"""
from test_framework.messages import
...
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2086524579)
nit: would prefer avoiding the literal by keeping `MAX_OP_RETURN_RELAY`, or at least document why you picked 99800.
<details>
<summary>git diff on c164064c26</summary>
```diff
diff --git a/test/functional/mempool_datacarrier.py b/test/functional/mempool_datacarrier.py
index 6347215b86..8eab4ce6b2 100755
--- a/test/functional/mempool_datacarrier.py
+++ b/test/functional/mempool_datacarrier.py
@@ -5,6 +5,7 @@
"""Test datacarrier functionality"""
from test_framework.messages import
...
💬 stickies-v commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2086497989)
nit: no strong view, and not important, but my preference would be to just remove the `std::optional` altogether. Simplifies fn signature and defines what zero means in a single location.
<details>
<summary>git diff on c164064c26</summary>
```diff
diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h
index d57dbb393f..4786aca67d 100644
--- a/src/kernel/mempool_options.h
+++ b/src/kernel/mempool_options.h
@@ -48,9 +48,9 @@ struct MemPoolOptions {
* type is de
...
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2086497989)
nit: no strong view, and not important, but my preference would be to just remove the `std::optional` altogether. Simplifies fn signature and defines what zero means in a single location.
<details>
<summary>git diff on c164064c26</summary>
```diff
diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h
index d57dbb393f..4786aca67d 100644
--- a/src/kernel/mempool_options.h
+++ b/src/kernel/mempool_options.h
@@ -48,9 +48,9 @@ struct MemPoolOptions {
* type is de
...
💬 stickies-v commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2086553680)
nit: I think it's strange to let one test handle another test's pre's, would just keep this in `test_block_max_weight`?
<details>
<summary>git diff on c164064c26</summary>
```diff
diff --git a/test/functional/mining_basic.py b/test/functional/mining_basic.py
index 21451a9aaf..415a370b06 100755
--- a/test/functional/mining_basic.py
+++ b/test/functional/mining_basic.py
@@ -186,9 +186,6 @@ class MiningTest(BitcoinTestFramework):
assert tx_below_min_feerate['txid'] not in
...
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2086553680)
nit: I think it's strange to let one test handle another test's pre's, would just keep this in `test_block_max_weight`?
<details>
<summary>git diff on c164064c26</summary>
```diff
diff --git a/test/functional/mining_basic.py b/test/functional/mining_basic.py
index 21451a9aaf..415a370b06 100755
--- a/test/functional/mining_basic.py
+++ b/test/functional/mining_basic.py
@@ -186,9 +186,6 @@ class MiningTest(BitcoinTestFramework):
assert tx_below_min_feerate['txid'] not in
...
💬 stickies-v commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2086563850)
nit: would make sense to actually use `test_framework.script.MAX_SCRIPT_ELEMENT_SIZE` here?
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2086563850)
nit: would make sense to actually use `test_framework.script.MAX_SCRIPT_ELEMENT_SIZE` here?
🚀 fanquake merged a pull request: "refactor: Remove unused HaveKey and HaveWatchOnly"
(https://github.com/bitcoin/bitcoin/pull/32476)
(https://github.com/bitcoin/bitcoin/pull/32476)
💬 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-2876150177)
What is the status of this? Do we need to add something to the docs?
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2876150177)
What is the status of this? Do we need to add something to the docs?
💬 fanquake commented on pull request "doc: update unix build doc with build flags":
(https://github.com/bitcoin/bitcoin/pull/32269#issuecomment-2876162598)
As mentioned, what you're adding is already present in this file, so if you're re-ordering things, you'll need to remove the second occurence.
(https://github.com/bitcoin/bitcoin/pull/32269#issuecomment-2876162598)
As mentioned, what you're adding is already present in this file, so if you're re-ordering things, you'll need to remove the second occurence.