💬 bitschmidty commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320079835)
> Popularity is not a sufficient reason alone, can you provide any of the underlying reasons why these users want to continue using this option?
Sure, from what Ive seen: Some miners want to use Bitcoin Core for block template building but dont want to include large op_returns. Relay nodes/node runners do not want to relay unconfirmed large op_returns.
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320079835)
> Popularity is not a sufficient reason alone, can you provide any of the underlying reasons why these users want to continue using this option?
Sure, from what Ive seen: Some miners want to use Bitcoin Core for block template building but dont want to include large op_returns. Relay nodes/node runners do not want to relay unconfirmed large op_returns.
🤔 Zero-1729 reviewed a pull request: "docs: Undeprecate datacarrier and datacarriersize configuration options"
(https://github.com/bitcoin/bitcoin/pull/33453#pullrequestreview-3253975016)
crACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
Agree with the rationale in the OP. This is a step in the right direction IMHO.
(https://github.com/bitcoin/bitcoin/pull/33453#pullrequestreview-3253975016)
crACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
Agree with the rationale in the OP. This is a step in the right direction IMHO.
💬 Kruwed commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320173852)
>Sure, from what Ive seen: Some miners want to use Bitcoin Core for block template building but don't want to include large op_returns. Relay nodes/node runners do not want to relay unconfirmed large op_returns.
Why should Bitcoin Core support a config option to perform targeted censorship of large OP_RETURNs?
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320173852)
>Sure, from what Ive seen: Some miners want to use Bitcoin Core for block template building but don't want to include large op_returns. Relay nodes/node runners do not want to relay unconfirmed large op_returns.
Why should Bitcoin Core support a config option to perform targeted censorship of large OP_RETURNs?
🤔 ismaelsadeeq reviewed a pull request: "rpc: fix getblock(header) returns target for tip"
(https://github.com/bitcoin/bitcoin/pull/33446#pullrequestreview-3254118873)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33446#pullrequestreview-3254118873)
Concept ACK
⚠️ ArmchairCryptologist opened an issue: "Documentation trap when using -bind in conjunction with (automatically configured) Tor hidden services"
(https://github.com/bitcoin/bitcoin/issues/33458)
To start with the conclusion: everything appears to work as _intended_, so this should probably not be considered a bug as such, but some logging and telemetry output could be improved to make things clearer.
If you use -bind to manually specify which (public) IPs to listen to, Bitcoin Core will also no longer create the (local) default socket for incoming Tor connections at 127.0.01:8334 unless you specify it manually in your list - this is probably working as intended. When automatically crea
...
(https://github.com/bitcoin/bitcoin/issues/33458)
To start with the conclusion: everything appears to work as _intended_, so this should probably not be considered a bug as such, but some logging and telemetry output could be improved to make things clearer.
If you use -bind to manually specify which (public) IPs to listen to, Bitcoin Core will also no longer create the (local) default socket for incoming Tor connections at 127.0.01:8334 unless you specify it manually in your list - this is probably working as intended. When automatically crea
...
💬 mzumsande commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2369524532)
thanks - I took that.
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2369524532)
thanks - I took that.
💬 mzumsande commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3320316739)
> This version doesn't work properly and produces the same issue it's trying to fix
Good catch, must have missed this during testsing! Changed to your suggestion with a `static`.
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3320316739)
> This version doesn't work properly and produces the same issue it's trying to fix
Good catch, must have missed this during testsing! Changed to your suggestion with a `static`.
💬 Raimo33 commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320316797)
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
I believe Bitcoin Core should remain neutral and allow **full configurability** at the policy level, while still providing the best defaults for most users. It is not Core’s role, or Knots’, or anyone’s to decide which options P2P participants may use.
Removing configurability from the reference implementation raises the barrier for non-technical users who want to run Bitcoin with their preferred settings. This restricts freedom of choice withou
...
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320316797)
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
I believe Bitcoin Core should remain neutral and allow **full configurability** at the policy level, while still providing the best defaults for most users. It is not Core’s role, or Knots’, or anyone’s to decide which options P2P participants may use.
Removing configurability from the reference implementation raises the barrier for non-technical users who want to run Bitcoin with their preferred settings. This restricts freedom of choice withou
...
🤔 polespinasa reviewed a pull request: "docs: Undeprecate datacarrier and datacarriersize configuration options"
(https://github.com/bitcoin/bitcoin/pull/33453#pullrequestreview-3254058148)
cNACK
As per [wikipedia definition](https://en.wikipedia.org/wiki/Deprecation):
_"Deprecation is the discouragement of use of something human-made, such as a linguistic term a proper name a feature, design, functionality, piece of code, or practice."_
This is exactly what was intended to do with the original (merged) PR. IMHO if we want to build software that we consider good for the users, as we stated in the [relay-policy post](https://bitcoincore.org/en/2025/06/06/relay-statement/), w
...
(https://github.com/bitcoin/bitcoin/pull/33453#pullrequestreview-3254058148)
cNACK
As per [wikipedia definition](https://en.wikipedia.org/wiki/Deprecation):
_"Deprecation is the discouragement of use of something human-made, such as a linguistic term a proper name a feature, design, functionality, piece of code, or practice."_
This is exactly what was intended to do with the original (merged) PR. IMHO if we want to build software that we consider good for the users, as we stated in the [relay-policy post](https://bitcoincore.org/en/2025/06/06/relay-statement/), w
...
💬 polespinasa commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#discussion_r2369422871)
Maybe just remove the `"expected to be removed in a future version"` sentence.
```c++
InitWarning(_("Options '-datacarrier' or '-datacarriersize' are set but are marked as deprecated and the use of it is discouraged."));
```
(https://github.com/bitcoin/bitcoin/pull/33453#discussion_r2369422871)
Maybe just remove the `"expected to be removed in a future version"` sentence.
```c++
InitWarning(_("Options '-datacarrier' or '-datacarriersize' are set but are marked as deprecated and the use of it is discouraged."));
```
💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2369478308)
as of "Rework RBF and TRUC validation" this check is now redundant I believe
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2369478308)
as of "Rework RBF and TRUC validation" this check is now redundant I believe
💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2369521026)
00c02b42c6199a6c46573304761056082490c7d4
While we're here: Suggestion to make it more clear as to the roles of the various TRUC checks and that everything works once dependencies are attached.
```Suggestion
// Either sibling eviction was needed, or package based check should always pass for single tx given SingleTRUCChecks already passed
Assume(ws.m_sibling_eviction || !PackageTRUCChecks(m_pool, ws.m_ptx, ws.m_vsize, {ws.m_ptx}, ws.m_parents));
```
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2369521026)
00c02b42c6199a6c46573304761056082490c7d4
While we're here: Suggestion to make it more clear as to the roles of the various TRUC checks and that everything works once dependencies are attached.
```Suggestion
// Either sibling eviction was needed, or package based check should always pass for single tx given SingleTRUCChecks already passed
Assume(ws.m_sibling_eviction || !PackageTRUCChecks(m_pool, ws.m_ptx, ws.m_vsize, {ws.m_ptx}, ws.m_parents));
```
💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2369593431)
I agree after some thought that txgraph-based checking is too difficult to do, at least for now. I can foresee some weird complications and I'm unsure I could even describe which series of steps would be correct vs our more bespoke approach currently which sidesteps it.
Re-arrangement of checks seems correct, will take a fresh look at the approach soon.
related: Documentation for `PreChecks` needs to be updated as it does not check package(cluster) limits any longer aside from TRUC checks.
...
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2369593431)
I agree after some thought that txgraph-based checking is too difficult to do, at least for now. I can foresee some weird complications and I'm unsure I could even describe which series of steps would be correct vs our more bespoke approach currently which sidesteps it.
Re-arrangement of checks seems correct, will take a fresh look at the approach soon.
related: Documentation for `PreChecks` needs to be updated as it does not check package(cluster) limits any longer aside from TRUC checks.
...
💬 Crypt-iQ commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320444838)
@ajtowns wrote:
> For comparison, the difficulty increase from early May to now (given the same hashrate) implies a decrease in revenue of 19.5%, more than 100x as much. I think it makes sense to set defaults to maximise even small amounts of revenue, but that's a "picking up pennies" saving, not a "small miners will be put out of business" situation.
Is the argument that poor compact block reconstruction rates leads to miner centralization overblown then if the loss in revenue will be dwa
...
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320444838)
@ajtowns wrote:
> For comparison, the difficulty increase from early May to now (given the same hashrate) implies a decrease in revenue of 19.5%, more than 100x as much. I think it makes sense to set defaults to maximise even small amounts of revenue, but that's a "picking up pennies" saving, not a "small miners will be put out of business" situation.
Is the argument that poor compact block reconstruction rates leads to miner centralization overblown then if the loss in revenue will be dwa
...
💬 mzumsande commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3320482300)
> This version doesn't work properly and produces the same issue it's trying to fix as every call to MakeWalletDatabase() within conditionals in VerifyWallets() (src/wallet/load.cpp), ends up also calling the SQLiteDatabase::Cleanup() in the destructor which does --g_sqlite_count.
Logging aside, I wonder if it's useful and intended to do all the existing setup including `sqlite3_initialize()`, followed by `sqlite3_shutdown()` multiple times during init for each wallet checked during `VerifyWa
...
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3320482300)
> This version doesn't work properly and produces the same issue it's trying to fix as every call to MakeWalletDatabase() within conditionals in VerifyWallets() (src/wallet/load.cpp), ends up also calling the SQLiteDatabase::Cleanup() in the destructor which does --g_sqlite_count.
Logging aside, I wonder if it's useful and intended to do all the existing setup including `sqlite3_initialize()`, followed by `sqlite3_shutdown()` multiple times during init for each wallet checked during `VerifyWa
...
👍 ryanofsky approved a pull request: "cli: Handle arguments that can be either JSON or string"
(https://github.com/bitcoin/bitcoin/pull/33230#pullrequestreview-3254240166)
Code review ACK aabf1f6093892d372544c8b5d55d260699dab69c. This should be a nice usability improvement. I left a suggestion for making the test stronger and confirming client and server types are consistent, and I think it would not be hard to implement, but it's not critical.
I do think this approach is a pretty reasonable way to avoid the need for calling `bitcoin-cli` with extra quotes around `ParseHashOrHeight` parameters here, and around addresses in #32468. And it's probably the best app
...
(https://github.com/bitcoin/bitcoin/pull/33230#pullrequestreview-3254240166)
Code review ACK aabf1f6093892d372544c8b5d55d260699dab69c. This should be a nice usability improvement. I left a suggestion for making the test stronger and confirming client and server types are consistent, and I think it would not be hard to implement, but it's not critical.
I do think this approach is a pretty reasonable way to avoid the need for calling `bitcoin-cli` with extra quotes around `ParseHashOrHeight` parameters here, and around addresses in #32468. And it's probably the best app
...
💬 ryanofsky commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2369570345)
> This is here because the test fails without it.
It seems like the test could be improved, but it would expand the PR a little. I think ideally type of `hash_or_height` `rollback` parameters server side would not be `NUM`, but a more accurate type like `NUM_OR_STR`. (#32468 could use `ARR_OR_STR`.)
The server could return this information back to the test either by modifying the current type column:
https://github.com/bitcoin/bitcoin/blob/34fefb633584ecd803b01209756f2bef412f1cb1/src/rp
...
(https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2369570345)
> This is here because the test fails without it.
It seems like the test could be improved, but it would expand the PR a little. I think ideally type of `hash_or_height` `rollback` parameters server side would not be `NUM`, but a more accurate type like `NUM_OR_STR`. (#32468 could use `ARR_OR_STR`.)
The server could return this information back to the test either by modifying the current type column:
https://github.com/bitcoin/bitcoin/blob/34fefb633584ecd803b01209756f2bef412f1cb1/src/rp
...
💬 furszy commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3320690729)
> > This version doesn't work properly and produces the same issue it's trying to fix as every call to MakeWalletDatabase() within conditionals in VerifyWallets() (src/wallet/load.cpp), ends up also calling the SQLiteDatabase::Cleanup() in the destructor which does --g_sqlite_count.
>
> Logging aside, I wonder if it's useful and intended to do all the existing setup including `sqlite3_initialize()`, followed by `sqlite3_shutdown()` multiple times during init for each wallet checked during `Ve
...
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3320690729)
> > This version doesn't work properly and produces the same issue it's trying to fix as every call to MakeWalletDatabase() within conditionals in VerifyWallets() (src/wallet/load.cpp), ends up also calling the SQLiteDatabase::Cleanup() in the destructor which does --g_sqlite_count.
>
> Logging aside, I wonder if it's useful and intended to do all the existing setup including `sqlite3_initialize()`, followed by `sqlite3_shutdown()` multiple times during init for each wallet checked during `Ve
...
💬 hodlinator commented on pull request "msvc: Update vcpkg manifest":
(https://github.com/bitcoin/bitcoin/pull/33408#issuecomment-3320698708)
ACK ef20c2d11d960bf915f88cdb2ceac2184e4aec10
Verified vcpkg tags / commit hashes.
nit: Discovered image blurring/smoothing worsened from master to PR. Here are two native Windows builds:
#### 947bed28fe62cd6c27baace5ae7c2b6955ee6379 (parent commit of PR on master, built natively on Windows)
<img width="433" height="349" alt="947bed28fe62cd6c27baace5ae7c2b6955ee6379" src="https://github.com/user-attachments/assets/8a78e493-4a36-424c-8716-3a3679f09c4c" />
#### ef20c2d11d960bf915f88cdb
...
(https://github.com/bitcoin/bitcoin/pull/33408#issuecomment-3320698708)
ACK ef20c2d11d960bf915f88cdb2ceac2184e4aec10
Verified vcpkg tags / commit hashes.
nit: Discovered image blurring/smoothing worsened from master to PR. Here are two native Windows builds:
#### 947bed28fe62cd6c27baace5ae7c2b6955ee6379 (parent commit of PR on master, built natively on Windows)
<img width="433" height="349" alt="947bed28fe62cd6c27baace5ae7c2b6955ee6379" src="https://github.com/user-attachments/assets/8a78e493-4a36-424c-8716-3a3679f09c4c" />
#### ef20c2d11d960bf915f88cdb
...
💬 pablomartin4btc commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3320719513)
> Logging aside, I wonder if it's useful and intended to do all the existing setup including `sqlite3_initialize()`, followed by `sqlite3_shutdown()` multiple times during init for each wallet checked during `VerifyWallets()`? Seems like the intention of `g_sqlite_count` is to do it only once?!
So, we have one sqlite (/ walletdb/ count) obj instance per wallet (/ wallet file), so during init we check first if there's a default wallet and then also check the wallets in the settings config (wha
...
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3320719513)
> Logging aside, I wonder if it's useful and intended to do all the existing setup including `sqlite3_initialize()`, followed by `sqlite3_shutdown()` multiple times during init for each wallet checked during `VerifyWallets()`? Seems like the intention of `g_sqlite_count` is to do it only once?!
So, we have one sqlite (/ walletdb/ count) obj instance per wallet (/ wallet file), so during init we check first if there's a default wallet and then also check the wallets in the settings config (wha
...