💬 glozow commented on issue "Ephemeral Dust 0-Fee Requirement Complexifies Downstream Protocols":
(https://github.com/bitcoin/bitcoin/issues/31938#issuecomment-2679432783)
Do we have stats on how many current-day anchors are hanging out in the UTXO set? I know they're effectively anyone-can-spend and theoretically can be cleaned up, but does that happen in practice?
(https://github.com/bitcoin/bitcoin/issues/31938#issuecomment-2679432783)
Do we have stats on how many current-day anchors are hanging out in the UTXO set? I know they're effectively anyone-can-spend and theoretically can be cleaned up, but does that happen in practice?
💬 TheBlueMatt commented on issue "Ephemeral Dust 0-Fee Requirement Complexifies Downstream Protocols":
(https://github.com/bitcoin/bitcoin/issues/31938#issuecomment-2679442360)
> So to summarize.
Yep!
> So this is kind of like removing the dust limit (apart from the rule that you can only have 1).
And that its only for anyone-can-spend/P2A.
> You're saying they're incentivized to do this so that nobody else is able to see the tx prior to confirmation (and thus can't snipe if from them)?
Correct, though that just is why we have to do the pay-to-fee in the `amt > dust` case.
> I have to ask: why would miners clean up 0-value outputs ever, other than to be nice?
> D
...
(https://github.com/bitcoin/bitcoin/issues/31938#issuecomment-2679442360)
> So to summarize.
Yep!
> So this is kind of like removing the dust limit (apart from the rule that you can only have 1).
And that its only for anyone-can-spend/P2A.
> You're saying they're incentivized to do this so that nobody else is able to see the tx prior to confirmation (and thus can't snipe if from them)?
Correct, though that just is why we have to do the pay-to-fee in the `amt > dust` case.
> I have to ask: why would miners clean up 0-value outputs ever, other than to be nice?
> D
...
💬 glozow commented on issue "Ephemeral Dust 0-Fee Requirement Complexifies Downstream Protocols":
(https://github.com/bitcoin/bitcoin/issues/31938#issuecomment-2679465093)
> > So to summarize.
>
> Yep!
>
> > So this is kind of like removing the dust limit (apart from the rule that you can only have 1).
>
> And that its only for anyone-can-spend/P2A.
Ohhh, that is different! So the request is to waive the 0-fee requirement specifically for p2a. This is a bit better, but I'm still not convinced that we can expect the outputs to be cleaned up. It's not like the anchors are going to be in "the dust range." They're always going to be exactly 0.
(https://github.com/bitcoin/bitcoin/issues/31938#issuecomment-2679465093)
> > So to summarize.
>
> Yep!
>
> > So this is kind of like removing the dust limit (apart from the rule that you can only have 1).
>
> And that its only for anyone-can-spend/P2A.
Ohhh, that is different! So the request is to waive the 0-fee requirement specifically for p2a. This is a bit better, but I'm still not convinced that we can expect the outputs to be cleaned up. It's not like the anchors are going to be in "the dust range." They're always going to be exactly 0.
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1968322091)
> If I understand it correctly, ...
Correct 👍 the general idea is to put a hard cap on the memory usage per peer. However, if a peer is using a lot more than the others simply because it is very useful, we don't penalize them until we run out of space.
We could do a `RegisterPeer` type of thing when the peer first connects, but I don't see any particular reason to do that extra step right now. Perhaps we can add this in the future, if we want to give peers a different reservation dependin
...
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1968322091)
> If I understand it correctly, ...
Correct 👍 the general idea is to put a hard cap on the memory usage per peer. However, if a peer is using a lot more than the others simply because it is very useful, we don't penalize them until we run out of space.
We could do a `RegisterPeer` type of thing when the peer first connects, but I don't see any particular reason to do that extra step right now. Perhaps we can add this in the future, if we want to give peers a different reservation dependin
...
💬 TheBlueMatt commented on issue "Ephemeral Dust 0-Fee Requirement Complexifies Downstream Protocols":
(https://github.com/bitcoin/bitcoin/issues/31938#issuecomment-2679490142)
I think it comes down to if block space is going to be free or not - in my view its kinda binary - if block space is never-free the dust concept kinda goes out the window because fees are material and tons of UTXOs are worthless, if block space is sometimes-free (bad news for bitcoin, but hey) then you just have to find one miner who wants to clean up crap and you're good.
(https://github.com/bitcoin/bitcoin/issues/31938#issuecomment-2679490142)
I think it comes down to if block space is going to be free or not - in my view its kinda binary - if block space is never-free the dust concept kinda goes out the window because fees are material and tons of UTXOs are worthless, if block space is sometimes-free (bad news for bitcoin, but hey) then you just have to find one miner who wants to clean up crap and you're good.
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1968340075)
My general impression is that, since these "spam" orphans are real, fee-paying transactions that end up in mempool, they are equally useful / equally deserve the space. I do think this represents a bound on how much total orphan volume we can handle, but I can't really see why the victim node should evict these orphans over the others, just because they only originate from 1 peer.
> Unfortunately we have no way of peers to communicate which orphans they claim are higher paying (to preferentia
...
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1968340075)
My general impression is that, since these "spam" orphans are real, fee-paying transactions that end up in mempool, they are equally useful / equally deserve the space. I do think this represents a bound on how much total orphan volume we can handle, but I can't really see why the victim node should evict these orphans over the others, just because they only originate from 1 peer.
> Unfortunately we have no way of peers to communicate which orphans they claim are higher paying (to preferentia
...
💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2679657115)
@janb84, thanks for the review.
> To consider; change title to "must find a fitting solution" in stead of "must find a good solution", but it's a very small nit.
I think "good" is just as descriptive as "fine" here although I really don't care either way about the title.
> If you search for e.g., the target amount, you might notice that the first few tests of this set were copied from the SRD tests. In the context of the CoinGrinder pull request, the tests were also used to demonstrate
...
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2679657115)
@janb84, thanks for the review.
> To consider; change title to "must find a fitting solution" in stead of "must find a good solution", but it's a very small nit.
I think "good" is just as descriptive as "fine" here although I really don't care either way about the title.
> If you search for e.g., the target amount, you might notice that the first few tests of this set were copied from the SRD tests. In the context of the CoinGrinder pull request, the tests were also used to demonstrate
...
💬 yancyribbens commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1968497012)
```suggestion
// j cannot be 0 or less; if it was, then there was necessarily nothing earlier which
```
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1968497012)
```suggestion
// j cannot be 0 or less; if it was, then there was necessarily nothing earlier which
```
💬 yancyribbens commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1968500577)
```
if it was, then there was necessarily nothing earlier which elem needs to be place before anymore, and place_before would be empty.
```
This comment seems jumbled and hard to understand. Is it possible to word this better?
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1968500577)
```
if it was, then there was necessarily nothing earlier which elem needs to be place before anymore, and place_before would be empty.
```
This comment seems jumbled and hard to understand. Is it possible to word this better?
💬 yancyribbens commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1968529011)
```suggestion
// Reuse a discarded node handle.
```
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1968529011)
```suggestion
// Reuse a discarded node handle.
```
🤔 Prabhat1308 reviewed a pull request: "test: add coverage for abandoning unconfirmed transaction"
(https://github.com/bitcoin/bitcoin/pull/31943#pullrequestreview-2638693713)
ACK [073a017](https://github.com/bitcoin/bitcoin/pull/31943/commits/073a017016e62c7d52562aa61d942024379c110d)
Covers the previously uncovered `TransactionCanBeAbandoned()` function increasing the coverage .
(https://github.com/bitcoin/bitcoin/pull/31943#pullrequestreview-2638693713)
ACK [073a017](https://github.com/bitcoin/bitcoin/pull/31943/commits/073a017016e62c7d52562aa61d942024379c110d)
Covers the previously uncovered `TransactionCanBeAbandoned()` function increasing the coverage .
💬 yancyribbens commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r1968625130)
```suggestion
// j cannot be 0 or less; if it was, then there was necessarily nothing earlier which
```
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r1968625130)
```suggestion
// j cannot be 0 or less; if it was, then there was necessarily nothing earlier which
```
💬 Dearfor commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2680811064)
👍
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2680811064)
👍
💬 maflcko commented on pull request "log,optimization: use original log string when no suspicious chars found":
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2680976216)
Tend toward NACK for now, as explained in the previous comments: https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2674257449 and https://github.com/bitcoin/bitcoin/pull/31923#pullrequestreview-2636342094
Given the added benchmarks, optimization seems to be the goal of this pull request. However, without a visible end-to-end improvement in any scenario (except in the added micro-bench), I fail to see why two benchmarks should be added and why the code should be changed.
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2680976216)
Tend toward NACK for now, as explained in the previous comments: https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2674257449 and https://github.com/bitcoin/bitcoin/pull/31923#pullrequestreview-2636342094
Given the added benchmarks, optimization seems to be the goal of this pull request. However, without a visible end-to-end improvement in any scenario (except in the added micro-bench), I fail to see why two benchmarks should be added and why the code should be changed.
💬 maflcko commented on pull request "log,optimization: use original log string when no suspicious chars found":
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2680995009)
Looking at https://corecheck.dev/bitcoin/bitcoin/pulls/31923 it seems that the `LogWith*` benchmarks are not shown at all? Looks like a bug where the list is truncated?
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2680995009)
Looking at https://corecheck.dev/bitcoin/bitcoin/pulls/31923 it seems that the `LogWith*` benchmarks are not shown at all? Looks like a bug where the list is truncated?
💬 maflcko commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#issuecomment-2681050852)
> What about the `IsArgSet` ones followed by `GetPathArg`?
Not sure what you mean, but `GetPathArg` does not return `std::optional<_>`, so I don't think the changes that are done here are applicable to it. There may or may not be cleanups related to `GetPathArg`, but my recommendation would be to create a separate, unrelated and independent issue or pull request to discuss or propse changes.
(https://github.com/bitcoin/bitcoin/pull/31896#issuecomment-2681050852)
> What about the `IsArgSet` ones followed by `GetPathArg`?
Not sure what you mean, but `GetPathArg` does not return `std::optional<_>`, so I don't think the changes that are done here are applicable to it. There may or may not be cleanups related to `GetPathArg`, but my recommendation would be to create a separate, unrelated and independent issue or pull request to discuss or propse changes.
💬 maflcko commented on issue "build: macOS fuzz instructions broken using latest macOS linker":
(https://github.com/bitcoin/bitcoin/issues/31049#issuecomment-2681063432)
Actually, re-reading the thread, it looks like the simplest solution would be to install lld and pass `-DCMAKE_CXX_FLAGS="-fuse-ld=lld"`, according to https://github.com/bitcoin/bitcoin/issues/31049#issuecomment-2624979670.
This avoids having to hard-code a specific version of clang in the docs (or ask the users to pick one), but no strong opinion.
(https://github.com/bitcoin/bitcoin/issues/31049#issuecomment-2681063432)
Actually, re-reading the thread, it looks like the simplest solution would be to install lld and pass `-DCMAKE_CXX_FLAGS="-fuse-ld=lld"`, according to https://github.com/bitcoin/bitcoin/issues/31049#issuecomment-2624979670.
This avoids having to hard-code a specific version of clang in the docs (or ask the users to pick one), but no strong opinion.
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1969325208)
> Curious why you include `RUST_BACKTRACE=1` in the recommended command line?
I just wanted the tool to behave similar to python when an assertion error is hit or when an "exception" is thrown. However, this can go away in the patch that polishes the error messages, which I plan to do in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1969325208)
> Curious why you include `RUST_BACKTRACE=1` in the recommended command line?
I just wanted the tool to behave similar to python when an assertion error is hit or when an "exception" is thrown. However, this can go away in the patch that polishes the error messages, which I plan to do in a follow-up.
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2681231443)
> `cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build util_tests `
> Hacky WIP pro-determinism patch resulting in pass
Thx, pushed a different fix/hack for this. I tested it on `util_string_tests`.
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2681231443)
> `cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build util_tests `
> Hacky WIP pro-determinism patch resulting in pass
Thx, pushed a different fix/hack for this. I tested it on `util_string_tests`.
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1969328357)
thx, pushed something
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1969328357)
thx, pushed something