💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1669052113)
it seems to me that `next` remains unchanged after clear:
```C++
auto next{node->second.Next()};
node->second.ClearFlags();
assert(next == node->second.Next());
node = next;
```
so if that's intentional, we can simplify to:
```C++
node->second.ClearFlags();
node = node->second.Next();
```
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1669052113)
it seems to me that `next` remains unchanged after clear:
```C++
auto next{node->second.Next()};
node->second.ClearFlags();
assert(next == node->second.Next());
node = next;
```
so if that's intentional, we can simplify to:
```C++
node->second.ClearFlags();
node = node->second.Next();
```
💬 mzumsande commented on issue "RPC `getblock` resulted in 500 and ReadBlockFromDisk: OpenBlockFile failed for FlatFilePos(nFile=-1, nPos=0)":
(https://github.com/bitcoin/bitcoin/issues/20978#issuecomment-2214874858)
If the block is not available anymore, we already throw a specific error: "Block not available (pruned data)": [rpc](https://github.com/bitcoin/bitcoin/blob/1f9d30744d32d24ad3128721cf5bd65a3f1543e8/src/rpc/blockchain.cpp#L586-L588) / [rest](https://github.com/bitcoin/bitcoin/blob/1f9d30744d32d24ad3128721cf5bd65a3f1543e8/src/rest.cpp#L310-L312)
So I think it would only be consistent to add a check for `BLOCK_HAVE_DATA`, not call `ReadRawBlockFromDisk` and return a similar error (e.g. "Block n
...
(https://github.com/bitcoin/bitcoin/issues/20978#issuecomment-2214874858)
If the block is not available anymore, we already throw a specific error: "Block not available (pruned data)": [rpc](https://github.com/bitcoin/bitcoin/blob/1f9d30744d32d24ad3128721cf5bd65a3f1543e8/src/rpc/blockchain.cpp#L586-L588) / [rest](https://github.com/bitcoin/bitcoin/blob/1f9d30744d32d24ad3128721cf5bd65a3f1543e8/src/rest.cpp#L310-L312)
So I think it would only be consistent to add a check for `BLOCK_HAVE_DATA`, not call `ReadRawBlockFromDisk` and return a similar error (e.g. "Block n
...
💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214881383)
@paplorinc With only a single link, how would you delete arbitrary map entries? Barring large hacks like putting the data in the next entry rather than the entry which the key points to, I don't see how you could delete entries in less than $\mathcal{O}(n)$ time.
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214881383)
@paplorinc With only a single link, how would you delete arbitrary map entries? Barring large hacks like putting the data in the next entry rather than the entry which the key points to, I don't see how you could delete entries in less than $\mathcal{O}(n)$ time.
💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1669095380)
this was unintentional. Disallowed it and added coverage.
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1669095380)
this was unintentional. Disallowed it and added coverage.
💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2214900126)
@theStack took test coverage suggestion, and yes "fees" is intentional since it will be in any net's address
@luke-jr I think @petertodd has stated what I would say. It's a pattern people can already do, and it's trivial and cheap to clean up.
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2214900126)
@theStack took test coverage suggestion, and yes "fees" is intentional since it will be in any net's address
@luke-jr I think @petertodd has stated what I would say. It's a pattern people can already do, and it's trivial and cheap to clean up.
💬 luke-jr commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2214922582)
>This proposal is merely a way of doing something that is already done - signed anchor outputs - in a more efficient way with less overall bytes consumed.
And at a reduced cost. Unless this proposal increases vsize to make it identical?
> The creating and spending of keyless anchor outputs is completely fixed, with no ability to encode any data. It has nothing to do with "spam".
What is the 0x4e73 for?
These outputs bloat the UTXO set, without sufficient value to incentivise cleanup
...
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2214922582)
>This proposal is merely a way of doing something that is already done - signed anchor outputs - in a more efficient way with less overall bytes consumed.
And at a reduced cost. Unless this proposal increases vsize to make it identical?
> The creating and spending of keyless anchor outputs is completely fixed, with no ability to encode any data. It has nothing to do with "spam".
What is the 0x4e73 for?
These outputs bloat the UTXO set, without sufficient value to incentivise cleanup
...
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214968788)
> With only a single link, how would you delete arbitrary map entries?
My naive thinking was that maybe we don't actually need random-element deletion. Based on how `ClearFlags` and `erase` are currently called from loops that already have the previous node, we could often call `ClearFlags(*prev)`.
The destructor is trickier. Maybe we can handle this in the `CCoinsViewCache` destructor instead, batch-destructing entries there, which would involve a single extra $\mathcal{O}(n)$ traversal.
I
...
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214968788)
> With only a single link, how would you delete arbitrary map entries?
My naive thinking was that maybe we don't actually need random-element deletion. Based on how `ClearFlags` and `erase` are currently called from loops that already have the previous node, we could often call `ClearFlags(*prev)`.
The destructor is trickier. Maybe we can handle this in the `CCoinsViewCache` destructor instead, batch-destructing entries there, which would involve a single extra $\mathcal{O}(n)$ traversal.
I
...
💬 petertodd commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2214969086)
On July 8, 2024 8:42:50 PM GMT+02:00, Luke Dashjr ***@***.***> wrote:
>>This proposal is merely a way of doing something that is already done - signed anchor outputs - in a more efficient way with less overall bytes consumed.
>
>And at a reduced cost. Unless this proposal increases vsize to make it identical?
The cost reduction is a natural one, in line with the byte reduction.
>> The creating and spending of keyless anchor outputs is completely fixed, with no ability to encode any data. It
...
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2214969086)
On July 8, 2024 8:42:50 PM GMT+02:00, Luke Dashjr ***@***.***> wrote:
>>This proposal is merely a way of doing something that is already done - signed anchor outputs - in a more efficient way with less overall bytes consumed.
>
>And at a reduced cost. Unless this proposal increases vsize to make it identical?
The cost reduction is a natural one, in line with the byte reduction.
>> The creating and spending of keyless anchor outputs is completely fixed, with no ability to encode any data. It
...
💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214976515)
@paplorinc When UTXOs are spent while FRESH, they get deleted from the cache; this is quite possibly the most significant performance optimization Bitcoin Core has during IBD, as it avoids the need for those UTXOs to ever end up on disk. It's done based on `COutPoint` lookup, not during iteration.
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214976515)
@paplorinc When UTXOs are spent while FRESH, they get deleted from the cache; this is quite possibly the most significant performance optimization Bitcoin Core has during IBD, as it avoids the need for those UTXOs to ever end up on disk. It's done based on `COutPoint` lookup, not during iteration.
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214993679)
Makes sense, thanks for explaining @sipa!
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214993679)
Makes sense, thanks for explaining @sipa!
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1669189198)
This code is gone.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1669189198)
This code is gone.
💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2215089061)
> What is the 0x4e73 for?
To be segwit, it needs to be a witness version + witness program(or p2sh nested segwit script). A witness program is 2 to 40 bytes long push. For this use-case we simply use size two. The specific bytes chosen are simply "cutesy", as they cannot be 0 so something else has to be chosen and have an associated address. As far as I know this is the only proposed size 2 witness program use case so far.
> These outputs bloat the UTXO set, without sufficient value to i
...
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2215089061)
> What is the 0x4e73 for?
To be segwit, it needs to be a witness version + witness program(or p2sh nested segwit script). A witness program is 2 to 40 bytes long push. For this use-case we simply use size two. The specific bytes chosen are simply "cutesy", as they cannot be 0 so something else has to be chosen and have an associated address. As far as I know this is the only proposed size 2 witness program use case so far.
> These outputs bloat the UTXO set, without sufficient value to i
...
🤔 mzumsande reviewed a pull request: "Assumeutxo: bugfix on loadtxoutset with a divergent chain + test"
(https://github.com/bitcoin/bitcoin/pull/29996#pullrequestreview-2164290570)
Code Review ACK 5b7f70ba2661a194a3c476b81e03785feddb4e1c
(https://github.com/bitcoin/bitcoin/pull/29996#pullrequestreview-2164290570)
Code Review ACK 5b7f70ba2661a194a3c476b81e03785feddb4e1c
🤔 fjahr reviewed a pull request: "assumeutxo: Don't load a snapshot if it's not in the best header chain"
(https://github.com/bitcoin/bitcoin/pull/30320#pullrequestreview-2164298309)
re-ACK ab478c5fa16427b496e8a93e4780770d4c270214
Confirmed only changes since last review were the rebase and addressing comments (per `git range-diff master 29ab88dfe1c43af3620480c99cba844aa414023c ab478c5fa16427b496e8a93e4780770d4c270214`).
(https://github.com/bitcoin/bitcoin/pull/30320#pullrequestreview-2164298309)
re-ACK ab478c5fa16427b496e8a93e4780770d4c270214
Confirmed only changes since last review were the rebase and addressing comments (per `git range-diff master 29ab88dfe1c43af3620480c99cba844aa414023c ab478c5fa16427b496e8a93e4780770d4c270214`).
💬 fjahr commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1669238451)
nit: seems the "snapshot" added before is back out, still not a block for me though
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1669238451)
nit: seems the "snapshot" added before is back out, still not a block for me though
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2215221618)
rebased on master, and s/anchor/dust/ everywhere
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2215221618)
rebased on master, and s/anchor/dust/ everywhere
💬 mzumsande commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1669272313)
ah, sorry, am on a different computer now and forgot to update the branch... Fixed now.
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1669272313)
ah, sorry, am on a different computer now and forgot to update the branch... Fixed now.
💬 fjahr commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#issuecomment-2215232338)
re-ACK b715a13d11c6ca7e928b2dee66edcc8c6c30a8d4
Just addressed my latest re-nit.
(https://github.com/bitcoin/bitcoin/pull/30320#issuecomment-2215232338)
re-ACK b715a13d11c6ca7e928b2dee66edcc8c6c30a8d4
Just addressed my latest re-nit.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1669280166)
I've changed it to "Find the best (highest-feerate, smallest among those in case of a tie) ancestor set among the remaining transactions.", because "best" is a bit more specific than highest-feerate.
Regarding the side note: that rule in `BlockAssembler` is an optimization, as it avoids looking at child transactions with higher ancestor feerate, because the ancestors will have been picked earlier anyway. The same optimization could be made in `AncestorCandidateFinder`, but because it's an $\m
...
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1669280166)
I've changed it to "Find the best (highest-feerate, smallest among those in case of a tie) ancestor set among the remaining transactions.", because "best" is a bit more specific than highest-feerate.
Regarding the side note: that rule in `BlockAssembler` is an optimization, as it avoids looking at child transactions with higher ancestor feerate, because the ancestors will have been picked earlier anyway. The same optimization could be made in `AncestorCandidateFinder`, but because it's an $\m
...
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1669281361)
I've added a comment that inc needs to be topological (a term which is defined in the `WorkItem` definition above).
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1669281361)
I've added a comment that inc needs to be topological (a term which is defined in the `WorkItem` definition above).