💬 achow101 commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687087037)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
`CheckEphemeralSpends` appears to only return txids, not wtxid, suggest renaming this variable:
```suggestion
const auto parent_txid = ephemeral_violation.value();
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687087037)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
`CheckEphemeralSpends` appears to only return txids, not wtxid, suggest renaming this variable:
```suggestion
const auto parent_txid = ephemeral_violation.value();
```
💬 achow101 commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687062365)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
nit: Use `contains`, here and elsewhere.
```suggestion
Assume(!map_tx_dust.contains(tx->GetHash()));
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687062365)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
nit: Use `contains`, here and elsewhere.
```suggestion
Assume(!map_tx_dust.contains(tx->GetHash()));
```
💬 achow101 commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687094839)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
It's not clear to me why there is a rule that a descendant must also spend the dust if they spent any other output from an ancestor. This rule does not seem to be mentioned anywhere in the PR description.
ISTM it would be easier and just as effective to check that all dust outputs end up being spent rather than this slightly convoluted check.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687094839)
In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"
It's not clear to me why there is a rule that a descendant must also spend the dust if they spent any other output from an ancestor. This rule does not seem to be mentioned anywhere in the PR description.
ISTM it would be easier and just as effective to check that all dust outputs end up being spent rather than this slightly convoluted check.
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687135318)
Imagine three transactions:
TxA, 0-fee with two outputs, one non-dust, one dust
TxB, spends non-dust
TxC, spends dust
All the dust is spent if `TxA+TxB+TxC` is accepted, but the mining template may just pick up `TxA+TxB` rather than the three "legal configurations:
0) None
1) `TxA+TxB+TxC`
2) `TxA+TxC`
By requiring the child transaction to sweep any dust from the parent txn, we ensure that there is a single child only, and this child is the only transaction possible for bringing f
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687135318)
Imagine three transactions:
TxA, 0-fee with two outputs, one non-dust, one dust
TxB, spends non-dust
TxC, spends dust
All the dust is spent if `TxA+TxB+TxC` is accepted, but the mining template may just pick up `TxA+TxB` rather than the three "legal configurations:
0) None
1) `TxA+TxB+TxC`
2) `TxA+TxC`
By requiring the child transaction to sweep any dust from the parent txn, we ensure that there is a single child only, and this child is the only transaction possible for bringing f
...
💬 pinheadmz commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2243815571)
@1440000bytes hello from your friendly neighborhood moderator. This is just a reminder to keep all discussions technical. Pull request comments should be focused on the code in question. Discussions about personal opinions, moderator or maintainer actions (including responses to this comment) can be opened [here](https://github.com/bitcoin-core/meta/issues)
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2243815571)
@1440000bytes hello from your friendly neighborhood moderator. This is just a reminder to keep all discussions technical. Pull request comments should be focused on the code in question. Discussions about personal opinions, moderator or maintainer actions (including responses to this comment) can be opened [here](https://github.com/bitcoin-core/meta/issues)
💬 1440000bytes commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2243823599)
> @1440000bytes hello from your friendly neighborhood moderator. This is just a reminder to keep all discussions technical. Pull request comments should be focused on the code in question. Discussions about personal opinions, moderator or maintainer actions (including responses to this comment) can be opened [here](https://github.com/bitcoin-core/meta/issues)
Code in question:
```diff
-static constexpr bool DEFAULT_MEMPOOL_FULL_RBF{false};
+static constexpr bool DEFAULT_MEMPOOL_FULL_RBF{
...
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2243823599)
> @1440000bytes hello from your friendly neighborhood moderator. This is just a reminder to keep all discussions technical. Pull request comments should be focused on the code in question. Discussions about personal opinions, moderator or maintainer actions (including responses to this comment) can be opened [here](https://github.com/bitcoin-core/meta/issues)
Code in question:
```diff
-static constexpr bool DEFAULT_MEMPOOL_FULL_RBF{false};
+static constexpr bool DEFAULT_MEMPOOL_FULL_RBF{
...
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687148241)
I can update the documentation reflecting this if you find the explanation compelling.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687148241)
I can update the documentation reflecting this if you find the explanation compelling.
💬 achow101 commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#discussion_r1687154537)
Release note fragments should have the PR number in them, and this is PR 30493, not 28132. The release version is not relevant for release note fragments.
(https://github.com/bitcoin/bitcoin/pull/30493#discussion_r1687154537)
Release note fragments should have the PR number in them, and this is PR 30493, not 28132. The release version is not relevant for release note fragments.
💬 achow101 commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687157629)
Ah, I see. Any child must spend the dust output, and I see that this is being enforced in the other variant too.
Additional documentation would be appreciated.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687157629)
Ah, I see. Any child must spend the dust output, and I see that this is being enforced in the other variant too.
Additional documentation would be appreciated.
💬 ryanofsky commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2243842400)
The PR description is a bit vague about how it is changing the AutoFile API.
Previously, the autofile API *allowed* but did not *require* AutoFile callers to check for failing `fclose` calls after writing. Now it requires that callers call the `fclose` method manually and explicitly use or discard its return value.
Checking for failing `fclose` calls is important because if a call fails, it probably indicates that there is a serious problem that the should trigger a log message or an alert
...
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2243842400)
The PR description is a bit vague about how it is changing the AutoFile API.
Previously, the autofile API *allowed* but did not *require* AutoFile callers to check for failing `fclose` calls after writing. Now it requires that callers call the `fclose` method manually and explicitly use or discard its return value.
Checking for failing `fclose` calls is important because if a call fails, it probably indicates that there is a serious problem that the should trigger a log message or an alert
...
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687164532)
> I still do think as long as you are going to update the loop to respect the length of the input (instead of blowing past it) it would be better update the loop to respect the length of the output as well, and not pointlessly read more data than can actually be used, as suggested https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683086730.
I tried making the loop counting `digits` actually respect the output length (`WIDTH*2` here) as well.
```C++
for (const char c : trimmed)
...
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687164532)
> I still do think as long as you are going to update the loop to respect the length of the input (instead of blowing past it) it would be better update the loop to respect the length of the output as well, and not pointlessly read more data than can actually be used, as suggested https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683086730.
I tried making the loop counting `digits` actually respect the output length (`WIDTH*2` here) as well.
```C++
for (const char c : trimmed)
...
🤔 mzumsande reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2192327884)
Code Review ACK d7f94e0fa9ec3641405e6909ab7da4e48865e840
(disclaimer: I'm not very experienced with the coins db but started to look into it more recently).
I did some testing on signet to ensure that the `dumptxoutset` data is not affected by this.
I'm in the process of doing some test runs on mainnet, but that'll take me a few more days.
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2192327884)
Code Review ACK d7f94e0fa9ec3641405e6909ab7da4e48865e840
(disclaimer: I'm not very experienced with the coins db but started to look into it more recently).
I did some testing on signet to ensure that the `dumptxoutset` data is not affected by this.
I'm in the process of doing some test runs on mainnet, but that'll take me a few more days.
💬 mzumsande commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687007975)
Did you consider setting `m_next` and `m_prev` be set to back to `nullptr` here? Doesn't seem strictly necessary but maybe a bit cleaner?
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687007975)
Did you consider setting `m_next` and `m_prev` be set to back to `nullptr` here? Doesn't seem strictly necessary but maybe a bit cleaner?
💬 mzumsande commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687159045)
While reviewing (and before reading this discussion) I also got stuck on this. Because of the expectations the naming raises, it seems like a potential footgun (future code may try to iterate over the linked list without intending to erase entries).
I think the side effect should be documented more clearly for the `Next()` function, maybe even rename to `NextAndMaybeErase()` or something similar.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687159045)
While reviewing (and before reading this discussion) I also got stuck on this. Because of the expectations the naming raises, it seems like a potential footgun (future code may try to iterate over the linked list without intending to erase entries).
I think the side effect should be documented more clearly for the `Next()` function, maybe even rename to `NextAndMaybeErase()` or something similar.
💬 mzumsande commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687135107)
I think that a call to `coins_view_cache.clear()` is necessary here since `BatchWrite()` doesn't erase entries anymore when `will_erase` is set.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687135107)
I think that a call to `coins_view_cache.clear()` is necessary here since `BatchWrite()` doesn't erase entries anymore when `will_erase` is set.
💬 mzumsande commented on pull request "validation: Improve, document and test logic for chains building on invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/30207#issuecomment-2243874664)
CI failure seems unrelated (#29897)
(https://github.com/bitcoin/bitcoin/pull/30207#issuecomment-2243874664)
CI failure seems unrelated (#29897)
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687182754)
It would have to be `coins_map.clear()`, which is passed up in the cursor to `coins_view_cache`. This block is equivalent to `Flush`, and `coins_view_cache` is the `base`.
However, `coins_map` goes out of scope right after this block and is not reused as in `Flush`, so clearing it is not necessary.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687182754)
It would have to be `coins_map.clear()`, which is passed up in the cursor to `coins_view_cache`. This block is equivalent to `Flush`, and `coins_view_cache` is the `base`.
However, `coins_map` goes out of scope right after this block and is not reused as in `Flush`, so clearing it is not necessary.
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687184627)
An earlier version of this PR that relied on checking for null did do this. I just think it's unnecessary work to set them to null, because access is guarded by `m_flags`.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687184627)
An earlier version of this PR that relied on checking for null did do this. I just think it's unnecessary work to set them to null, because access is guarded by `m_flags`.
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687185915)
This comment thread is pointing to an out of date version of the PR. There's more documentation on this struct and methods now, as well as changing a variable and field name. Maybe it is more clear now?
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687185915)
This comment thread is pointing to an out of date version of the PR. There's more documentation on this struct and methods now, as well as changing a variable and field name. Maybe it is more clear now?
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2243900791)
> This is a helpful summary, but not 100% accurate. `uint256S()` does currently require a terminator, but it doesn't have to be a `\0` terminator. Any character other than `01234567890ABCDEFabcdef` will work as a terminator and stop reading the string.
(Updated summary slightly).
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2243900791)
> This is a helpful summary, but not 100% accurate. `uint256S()` does currently require a terminator, but it doesn't have to be a `\0` terminator. Any character other than `01234567890ABCDEFabcdef` will work as a terminator and stop reading the string.
(Updated summary slightly).