🤔 murchandamus reviewed a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2905753533)
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2905753533)
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
💬 murchandamus commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2132689937)
I was a bit surprised that we accept this output script. I thought that the OP_RETURN opcode is to be followed by a push with the length of the data payload, but it looks like you are just writing raw data right after OP_RETURN. Could you clarify why no push is necessary for this to be accepted?
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2132689937)
I was a bit surprised that we accept this output script. I thought that the OP_RETURN opcode is to be followed by a push with the length of the data payload, but it looks like you are just writing raw data right after OP_RETURN. Could you clarify why no push is necessary for this to be accepted?
💬 murchandamus commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2132696711)
Nit:
```suggestion
- The default value for `-datacarriersize` is increased to 100,000 which effectively uncaps the limit (as the maximum transaction size limit will be hit first). It can be overridden with -datacarriersize=83 to revert to the limit enforced in previous versions. The `-datacarrier` and `-datacarriersize` options have been marked as deprecated and are expected to be removed in a future release. (#32406)
```
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2132696711)
Nit:
```suggestion
- The default value for `-datacarriersize` is increased to 100,000 which effectively uncaps the limit (as the maximum transaction size limit will be hit first). It can be overridden with -datacarriersize=83 to revert to the limit enforced in previous versions. The `-datacarrier` and `-datacarriersize` options have been marked as deprecated and are expected to be removed in a future release. (#32406)
```
💬 glozow commented on pull request "[28.x] 28.2rc2":
(https://github.com/bitcoin/bitcoin/pull/32684#discussion_r2132696243)
Are we missing #32003 and #32439?
(https://github.com/bitcoin/bitcoin/pull/32684#discussion_r2132696243)
Are we missing #32003 and #32439?
💬 darosior commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2132723838)
It's never executed and therefore does not have to be valid Script.
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2132723838)
It's never executed and therefore does not have to be valid Script.
💬 theStack commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2132761085)
@murchandamus: The test framework's CScript class automatically generates data pushes for byte-strings that are passed in the list, e.g.:
```
$ python3
Python 3.12.9 (main, Apr 9 2025, 12:59:26) [Clang 16.0.6 ] on openbsd7
Type "help", "copyright", "credits" or "license" for more information.
>>> from test_framework.script import CScript, OP_RETURN
>>> s = CScript([OP_RETURN, b'meh'])
>>> s.hex()
'6a036d6568'
```
> It's never executed and therefore does not have to be valid Script.
...
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2132761085)
@murchandamus: The test framework's CScript class automatically generates data pushes for byte-strings that are passed in the list, e.g.:
```
$ python3
Python 3.12.9 (main, Apr 9 2025, 12:59:26) [Clang 16.0.6 ] on openbsd7
Type "help", "copyright", "credits" or "license" for more information.
>>> from test_framework.script import CScript, OP_RETURN
>>> s = CScript([OP_RETURN, b'meh'])
>>> s.hex()
'6a036d6568'
```
> It's never executed and therefore does not have to be valid Script.
...
💬 murchandamus commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2132781720)
Ah thanks, @theStack!
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2132781720)
Ah thanks, @theStack!
💬 lordnakamoto commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2950417953)
Concept NACK. I have to disagree with the the concept of removing user control over mempool policies and the potential for massive increases in blockchain bloat. The 83-byte limit isn't arbitrary - it's there to prevent people from turning Bitcoin into a data storage service. We need to think of the possible worst case scenarios of this change, e.g. data hoarders filling up blocks with data, pushing out actual Bitcoin payments. The fee market gets completely screwed up when you mix payment trans
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2950417953)
Concept NACK. I have to disagree with the the concept of removing user control over mempool policies and the potential for massive increases in blockchain bloat. The 83-byte limit isn't arbitrary - it's there to prevent people from turning Bitcoin into a data storage service. We need to think of the possible worst case scenarios of this change, e.g. data hoarders filling up blocks with data, pushing out actual Bitcoin payments. The fee market gets completely screwed up when you mix payment trans
...
💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2132793068)
> I wonder what you think of e.g. making a bespoke class that houses m_txos and mapWallet where RefreshTXOsFromTx, RemoveTx etc. would live, and one could always be sure that the wallet's tracked transactions and tracked outputs always stay in sync. I admit that this scatters the code still, just in another way, but I wonder if at this size, we have to start more aggressively encapsulating functionality.
I don't think that makes sense to do as it would end up being just another wrapper around
...
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2132793068)
> I wonder what you think of e.g. making a bespoke class that houses m_txos and mapWallet where RefreshTXOsFromTx, RemoveTx etc. would live, and one could always be sure that the wallet's tracked transactions and tracked outputs always stay in sync. I admit that this scatters the code still, just in another way, but I wonder if at this size, we have to start more aggressively encapsulating functionality.
I don't think that makes sense to do as it would end up being just another wrapper around
...
💬 BitcoinMechanic commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2950473321)
I'd really appreciate it if @DrahtBot was manually updated to reflect my Concept NACK - currently it says I approve and I'd rather not go down in history as having been in favour of this terrible idea.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2950473321)
I'd really appreciate it if @DrahtBot was manually updated to reflect my Concept NACK - currently it says I approve and I'd rather not go down in history as having been in favour of this terrible idea.
💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2132803868)
Don't remember. Can remove if I need to retouch.
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2132803868)
Don't remember. Can remove if I need to retouch.
💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2132809017)
It would make more sense to drop unconfirmed balances when min_depth > 0 rather than include less than min_depth as a separate balance. Regardless, this PR is not trying to change user facing behavior, so this is out of scope.
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2132809017)
It would make more sense to drop unconfirmed balances when min_depth > 0 rather than include less than min_depth as a separate balance. Regardless, this PR is not trying to change user facing behavior, so this is out of scope.
💬 achow101 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2950505766)
> I'd really appreciate it if @DrahtBot was manually updated to reflect my Concept NACK
You can fix it yourself, as it clearly states in the comment:
> If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2950505766)
> I'd really appreciate it if @DrahtBot was manually updated to reflect my Concept NACK
You can fix it yourself, as it clearly states in the comment:
> If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
💬 furszy commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r2132820596)
In b3e2e1970e46bcfbe19d4d421bb6866ceab40ca0:
This doesn't seem to be correct. The last header is being set to the block we just disconnected. Also, it is in the wrong commit.
(no need to fix it here, I'm cherry-picking this commit into another branch to push it as a separate PR anyway).
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r2132820596)
In b3e2e1970e46bcfbe19d4d421bb6866ceab40ca0:
This doesn't seem to be correct. The last header is being set to the block we just disconnected. Also, it is in the wrong commit.
(no need to fix it here, I'm cherry-picking this commit into another branch to push it as a separate PR anyway).
💬 ryanofsky commented on pull request "refactor,test: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#issuecomment-2950665350)
> I don't think we should make a major overhaul to tricky consensus-critical code that was never touched in 15 years
@darosior I almost wonder if we are looking at the same PR. The current set of changes seem pretty trivial and are certainly not a major overhaul.
@l0rinc since the general arguments darosior's making are worth taking seriously and apply to a wide variety of changes maybe including the changes made here, it could make sense to separate the concerns more by opening a differen
...
(https://github.com/bitcoin/bitcoin/pull/32532#issuecomment-2950665350)
> I don't think we should make a major overhaul to tricky consensus-critical code that was never touched in 15 years
@darosior I almost wonder if we are looking at the same PR. The current set of changes seem pretty trivial and are certainly not a major overhaul.
@l0rinc since the general arguments darosior's making are worth taking seriously and apply to a wide variety of changes maybe including the changes made here, it could make sense to separate the concerns more by opening a differen
...
💬 murchandamus commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2950697976)
@BitcoinMechanic: I’m not sure what you expect DrahtBot to do, but it just parses comments and links to the last comment per user that contains a relevant string. So, when you make comments like this, what do you think should happen?
You fixed it yourself, congrats:
@BitcoinMechanic: I’m not sure what you expect DrahtBot to do, but it just parses comments and links to the last comment per user that contains a relevant string. So, when you make comments like this, what do you think should happen?
You fixed it yourself, congrats:
![image](https://github.com/user-attachments/
...
👍 darosior approved a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2906075470)
Concept ACK a189d636184b1c28fa4a325b56c1fab8f44527b1.
I prefer the approach from #32359 because i believe that exposing the option is at best misleading to users. However i am fine with any approach (including this one) which gets rid of the poor incentive unnecessarily set by the existing limit.
Since this change has created a significant amount of confusion among bitcoiners, i have compiled a list of objections and concerns raised by the community and addressed them all [in a single post
...
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2906075470)
Concept ACK a189d636184b1c28fa4a325b56c1fab8f44527b1.
I prefer the approach from #32359 because i believe that exposing the option is at best misleading to users. However i am fine with any approach (including this one) which gets rid of the poor incentive unnecessarily set by the existing limit.
Since this change has created a significant amount of confusion among bitcoiners, i have compiled a list of objections and concerns raised by the community and addressed them all [in a single post
...
📝 furszy opened a pull request: "index: move disk read lookups to base class"
(https://github.com/bitcoin/bitcoin/pull/32694)
Combining common refactors from #24230 and #26966, aiming to move both efforts forward while reducing their size and review burden.
Broadly, #24230 focuses on enabling indexes to run in a separate process, and #26966 aims to parallelize the indexes initial synchronization process. A shared prerequisite for both is ensuring that only the base index class interacts with the node’s chain internals - child index classes should instead operate solely through chain events.
This PR moves disk rea
...
(https://github.com/bitcoin/bitcoin/pull/32694)
Combining common refactors from #24230 and #26966, aiming to move both efforts forward while reducing their size and review burden.
Broadly, #24230 focuses on enabling indexes to run in a separate process, and #26966 aims to parallelize the indexes initial synchronization process. A shared prerequisite for both is ensuring that only the base index class interacts with the node’s chain internals - child index classes should instead operate solely through chain events.
This PR moves disk rea
...
💬 darosior commented on pull request "refactor,test: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#issuecomment-2950744430)
@ryanofsky Maybe "overhaul" isn't the right term if it implies that a change is necessarily huge, and is should have used "refactoring" instead. Regardless, my point stands.
(https://github.com/bitcoin/bitcoin/pull/32532#issuecomment-2950744430)
@ryanofsky Maybe "overhaul" isn't the right term if it implies that a change is necessarily huge, and is should have used "refactoring" instead. Regardless, my point stands.
💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2132884946)
Since we are no longer using `CachedTxGetAvailableCredit` and `CachedTxGetImmautreCredit`, we have to first check whether the output is an immature coinbase. Otherwise immature coinbases will be included in the trusted balances which is incorrect.
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2132884946)
Since we are no longer using `CachedTxGetAvailableCredit` and `CachedTxGetImmautreCredit`, we have to first check whether the output is an immature coinbase. Otherwise immature coinbases will be included in the trusted balances which is incorrect.