💬 petertodd commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1656715854)
> From CI:
>
> ```
> test/functional/mempool_datacarrier.py:31: error: Bracketed expression "[...]" is not valid as a type [valid-type]
> test/functional/mempool_datacarrier.py:31: note: Did you mean "List[...]"?
> Found 1 error in 1 file (checked 269 source files)
> ^---- failure generated from lint-python.py
> ```
> From CI:
>
> ```
> test/functional/mempool_datacarrier.py:31: error: Bracketed expression "[...]" is not valid as a type [valid-type]
> test/functional/mempoo
...
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1656715854)
> From CI:
>
> ```
> test/functional/mempool_datacarrier.py:31: error: Bracketed expression "[...]" is not valid as a type [valid-type]
> test/functional/mempool_datacarrier.py:31: note: Did you mean "List[...]"?
> Found 1 error in 1 file (checked 269 source files)
> ^---- failure generated from lint-python.py
> ```
> From CI:
>
> ```
> test/functional/mempool_datacarrier.py:31: error: Bracketed expression "[...]" is not valid as a type [valid-type]
> test/functional/mempoo
...
💬 willcl-ark commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1656717430)
I’m on mobile and didn’t check the pull, but do you need to `from typing import List` too?
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1656717430)
I’m on mobile and didn’t check the pull, but do you need to `from typing import List` too?
⚠️ Billybeast01 opened an issue: "Flash bitcoin"
(https://github.com/bitcoin/bitcoin/issues/28182)
Flash bitcoin
(https://github.com/bitcoin/bitcoin/issues/28182)
Flash bitcoin
💬 Sjors commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#discussion_r1278293244)
In transaction_tests it's either the same number of +-1 which is confusing (can be solved with a comment), but I see in other places you made it 100 which is indeed more clear.
(https://github.com/bitcoin/bitcoin/pull/28130#discussion_r1278293244)
In transaction_tests it's either the same number of +-1 which is confusing (can be solved with a comment), but I see in other places you made it 100 which is indeed more clear.
💬 Sjors commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1656723149)
> So frankly, at this point I don't care to split it up into two pull-reqs for such a minor reason.
Making a significant change to default policy is not a minor reason.
> is paying me $1000 for 10 hours work to do this pull-req
That's great, but others have to spend hours reviewing this stuff, plus occasionally having to deal with the brigading. I'm more likely to do that for a change that keeps the default, but I'm not the only one who can review of course.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1656723149)
> So frankly, at this point I don't care to split it up into two pull-reqs for such a minor reason.
Making a significant change to default policy is not a minor reason.
> is paying me $1000 for 10 hours work to do this pull-req
That's great, but others have to spend hours reviewing this stuff, plus occasionally having to deal with the brigading. I'm more likely to do that for a change that keeps the default, but I'm not the only one who can review of course.
✅ fanquake closed an issue: "Flash bitcoin"
(https://github.com/bitcoin/bitcoin/issues/28182)
(https://github.com/bitcoin/bitcoin/issues/28182)
:lock: fanquake locked an issue: "Flash bitcoin"
(https://github.com/bitcoin/bitcoin/issues/28182)
(https://github.com/bitcoin/bitcoin/issues/28182)
💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1656730905)
We'll be dropping support for Cirrus CI dockerfiles anyway within the next 30 days, going to full VMs (either on GitHub Actions CI, or on self-hosted machines), so I think `chattr` should work fine, if this pull can wait for a few more days.
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1656730905)
We'll be dropping support for Cirrus CI dockerfiles anyway within the next 30 days, going to full VMs (either on GitHub Actions CI, or on self-hosted machines), so I think `chattr` should work fine, if this pull can wait for a few more days.
💬 MarcoFalke commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1656733618)
Unrelated to this thread, but I am moving the Linux stuff to self-hosted runners (like the "previous releases" task already is). Starting in https://github.com/bitcoin/bitcoin/pull/28161 with the `ASan` task, which needs a dedicated Ubuntu Lunar VM.
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1656733618)
Unrelated to this thread, but I am moving the Linux stuff to self-hosted runners (like the "previous releases" task already is). Starting in https://github.com/bitcoin/bitcoin/pull/28161 with the `ASan` task, which needs a dedicated Ubuntu Lunar VM.
💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1656734972)
Cool. Is there an open PR for that yet? Plz add me as reviewer!
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1656734972)
Cool. Is there an open PR for that yet? Plz add me as reviewer!
💬 Sjors commented on pull request "validation: add missing insert to m_dirty_blockindex":
(https://github.com/bitcoin/bitcoin/pull/27905#issuecomment-1656738063)
Should we make `nStatus` private and only allow writing to it through a helper method that makes it dirty?
(https://github.com/bitcoin/bitcoin/pull/27905#issuecomment-1656738063)
Should we make `nStatus` private and only allow writing to it through a helper method that makes it dirty?
💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1656744733)
Sure, https://github.com/bitcoin/bitcoin/pull/28161 is the fist step. It won't make a difference for `chattr`, because it is just moving one VM to another VM, but it is the first step.
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1656744733)
Sure, https://github.com/bitcoin/bitcoin/pull/28161 is the fist step. It won't make a difference for `chattr`, because it is just moving one VM to another VM, but it is the first step.
🤔 Sjors reviewed a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1553292155)
Comments around 768690b7ce551cd403f8e2a099372915f6022ad4.
It would be nice if the documentation for `setBlockIndexCandidates` and `m_blocks_unlinked` explained their purpose (as opposed to merely describing what's in them).
```cpp
/**
* The set of all CBlockIndex entries that are - or recently
* were - candidates for a new tip. These are as good as our
* current tip or better, and must either have BLOCK_VALID_TRANSACTIONS
* (for itself and all ancestors) *or* BLOCK_ASSUMED_VALID (if
...
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1553292155)
Comments around 768690b7ce551cd403f8e2a099372915f6022ad4.
It would be nice if the documentation for `setBlockIndexCandidates` and `m_blocks_unlinked` explained their purpose (as opposed to merely describing what's in them).
```cpp
/**
* The set of all CBlockIndex entries that are - or recently
* were - candidates for a new tip. These are as good as our
* current tip or better, and must either have BLOCK_VALID_TRANSACTIONS
* (for itself and all ancestors) *or* BLOCK_ASSUMED_VALID (if
...
💬 Sjors commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278308074)
768690b7ce551cd403f8e2a099372915f6022ad4: Maybe clarify that initially this only applies to the snapshot chainstate, which uses the base block as its starting point. For the background chainstate, as well an active chainstate not created from a snapshot, `GetSnapshotBaseBlock()` is `nullptr` so the special-case won't apply. But the background state will consider it a candidate after download.
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278308074)
768690b7ce551cd403f8e2a099372915f6022ad4: Maybe clarify that initially this only applies to the snapshot chainstate, which uses the base block as its starting point. For the background chainstate, as well an active chainstate not created from a snapshot, `GetSnapshotBaseBlock()` is `nullptr` so the special-case won't apply. But the background state will consider it a candidate after download.
💬 Sjors commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278312526)
768690b7ce551cd403f8e2a099372915f6022ad4: this had me confused, but it's because the test considers a scenario where the snapshot base block has been downloaded. It could be nice to include the moment _before_ it's downloaded where this should be `0`.
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278312526)
768690b7ce551cd403f8e2a099372915f6022ad4: this had me confused, but it's because the test considers a scenario where the snapshot base block has been downloaded. It could be nice to include the moment _before_ it's downloaded where this should be `0`.
💬 Sjors commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278308558)
768690b7ce551cd403f8e2a099372915f6022ad4
```cpp
//!
//! - Run LoadBlockIndex() and ensure that setBlockIndexCandidates of the first ...
```
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278308558)
768690b7ce551cd403f8e2a099372915f6022ad4
```cpp
//!
//! - Run LoadBlockIndex() and ensure that setBlockIndexCandidates of the first ...
```
💬 petertodd commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1656763717)
> > So frankly, at this point I don't care to split it up into two pull-reqs for such a minor reason.
>
> Making a significant change to default policy is not a minor reason.
This is not a significant change. Large amounts of data are already published in the bitcoin blockchain in a variety of ways. Heck, `-permitbaremultisig` is still allowed by default, and people still use it to publish data in the form of spendable and even unspendable outputs. And unlike OP_Return outputs, bare multis
...
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1656763717)
> > So frankly, at this point I don't care to split it up into two pull-reqs for such a minor reason.
>
> Making a significant change to default policy is not a minor reason.
This is not a significant change. Large amounts of data are already published in the bitcoin blockchain in a variety of ways. Heck, `-permitbaremultisig` is still allowed by default, and people still use it to publish data in the form of spendable and even unspendable outputs. And unlike OP_Return outputs, bare multis
...
💬 petertodd commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#discussion_r1278334958)
I reworked those unit tests with better comments and clearer constants, and expanded the test coverage a bit.
(https://github.com/bitcoin/bitcoin/pull/28130#discussion_r1278334958)
I reworked those unit tests with better comments and clearer constants, and expanded the test coverage a bit.
💬 petertodd commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1656787140)
@willcl-ark
> I’m on mobile and didn’t check the pull, but do you need to `from typing import List` too?
Thanks! I believe that fixed the issue.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1656787140)
@willcl-ark
> I’m on mobile and didn’t check the pull, but do you need to `from typing import List` too?
Thanks! I believe that fixed the issue.
👍 Sjors approved a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1553302075)
Code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9.
My comments above can all wait for a followup, if any.
I'd like to take this for a spin in a rebased #27596 with `-checkblockindex` to see if we didn't miss anything (without pruning, since that has a clear TODO).
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1553302075)
Code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9.
My comments above can all wait for a followup, if any.
I'd like to take this for a spin in a rebased #27596 with `-checkblockindex` to see if we didn't miss anything (without pruning, since that has a clear TODO).