💬 TheCharlatan commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2083272753)
Nit: `s/pqueue/queue/`
The relevant lines are touched already, so maybe just do it in the same commit?
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2083272753)
Nit: `s/pqueue/queue/`
The relevant lines are touched already, so maybe just do it in the same commit?
💬 1440000bytes commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2869146364)
This pull request won't get merged. If you really have any point that is worth considering please do it in #32406
I might get banned for this comment because Bitcoin Core contributors cannot read disagreements or dissent here or on other forums.
However, I would like to read your comments in #32406 if they make sense.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2869146364)
This pull request won't get merged. If you really have any point that is worth considering please do it in #32406
I might get banned for this comment because Bitcoin Core contributors cannot read disagreements or dissent here or on other forums.
However, I would like to read your comments in #32406 if they make sense.
💬 iicuriosity commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2869147520)
Concept NACK for the same reason as #32359. Why would you remove a feature with a valid use case?
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2869147520)
Concept NACK for the same reason as #32359. Why would you remove a feature with a valid use case?
🤔 monlovesmango reviewed a pull request: "cluster mempool: add txgraph diagrams/mining/eviction"
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2830971855)
ACK 7208ec2f1f0b9d72e1b4248dae9adbcc5ab625f7
All my other comments are just small nits.
Sorry it took a while to do my review, just wanted to make sure I actually felt like I had a good understanding of all the changes (which took a while).
Unrelated to this PR and more of an FYI, in src/test/fuzz/txgraph.cpp `pick_fn` I think that:
```c
size_t choices = tx_count[0] + sims[0].removed.size() + 1;
```
can just be:
```c
size_t choices = tx_count[0] + sims[0].removed.size();
```
an
...
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2830971855)
ACK 7208ec2f1f0b9d72e1b4248dae9adbcc5ab625f7
All my other comments are just small nits.
Sorry it took a while to do my review, just wanted to make sure I actually felt like I had a good understanding of all the changes (which took a while).
Unrelated to this PR and more of an FYI, in src/test/fuzz/txgraph.cpp `pick_fn` I think that:
```c
size_t choices = tx_count[0] + sims[0].removed.size() + 1;
```
can just be:
```c
size_t choices = tx_count[0] + sims[0].removed.size();
```
an
...
💬 monlovesmango commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2083253745)
Doesn't matter but could just return:
```suggestion
if (entry.m_main_chunkindex_iterator == m_main_chunkindex.end()) return
```
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2083253745)
Doesn't matter but could just return:
```suggestion
if (entry.m_main_chunkindex_iterator == m_main_chunkindex.end()) return
```
💬 monlovesmango commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2083253764)
Why doesn't `DeleteCluster` check for `m_main_chunkindex_observers == 0` like here and in `UnlinkRef`?
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2083253764)
Why doesn't `DeleteCluster` check for `m_main_chunkindex_observers == 0` like here and in `UnlinkRef`?
💬 monlovesmango commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2083273290)
This is pretty much identical to the logic in `CompareMainOrder`, should this be encapsulated in a single place as the source of truth (rather than having to ensure they stay equivalent)?
Unrelated to this particular PR, but what is the intended caller/use case for `CompareMainOrder`? Is the reason `CompareMainOrder` calls `MakeAcceptable` while `ChunkOrder` does not simply because we assume its already been called when `ChunkOrder` is invoked?
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2083273290)
This is pretty much identical to the logic in `CompareMainOrder`, should this be encapsulated in a single place as the source of truth (rather than having to ensure they stay equivalent)?
Unrelated to this particular PR, but what is the intended caller/use case for `CompareMainOrder`? Is the reason `CompareMainOrder` calls `MakeAcceptable` while `ChunkOrder` does not simply because we assume its already been called when `ChunkOrder` is invoked?
💬 monlovesmango commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2083236876)
Perhaps mention that `GetCurrentChunk` should only be called once (which is unusual for a getter), even though I can't think of an scenario where you would actually want to call this more than once.
However I think the best thing to do is actually not use `m_cur_cluster` for dual purposes and instead have a dedicated variable that tracks when singletons and last chunks don't need to be added to `m_excluded_clusters`. This would be a tad more comprehensible and there would be no concerns with
...
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2083236876)
Perhaps mention that `GetCurrentChunk` should only be called once (which is unusual for a getter), even though I can't think of an scenario where you would actually want to call this more than once.
However I think the best thing to do is actually not use `m_cur_cluster` for dual purposes and instead have a dedicated variable that tracks when singletons and last chunks don't need to be added to `m_excluded_clusters`. This would be a tad more comprehensible and there would be no concerns with
...
💬 polespinasa commented on pull request "rpc:generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2869169332)
> Just parse an array first and if that fails parse a string.
Oh right, that's an option. I will let it as it is for now to see other contributors' opinions.
May take your approach later.
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2869169332)
> Just parse an array first and if that fails parse a string.
Oh right, that's an option. I will let it as it is for now to see other contributors' opinions.
May take your approach later.
💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2083296383)
> You can recreate the failures
Thanks a lot @fanquake, I managed to reproduce it this way (seeing that https://github.com/l0rinc/bitcoin/pull/10 was failing, I was trying to run them manually instead of relying on the ci scripts).
> why it isn't needed (or rather since when)
Thanks @maflcko for insisting, I have changed the order of commits to place the suppression removal at the very end since we still had a few failures for the first few commits - it's easier to remove them all at th
...
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2083296383)
> You can recreate the failures
Thanks a lot @fanquake, I managed to reproduce it this way (seeing that https://github.com/l0rinc/bitcoin/pull/10 was failing, I was trying to run them manually instead of relying on the ci scripts).
> why it isn't needed (or rather since when)
Thanks @maflcko for insisting, I have changed the order of commits to place the suppression removal at the very end since we still had a few failures for the first few commits - it's easier to remove them all at th
...
💬 supertestnet commented on pull request "rpc:generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2869219420)
Can a PSBT be constructed with only outputs, no inputs? If so, then maybe `generateblock` could accept a psbt or a string as an argument. If it is a psbt, it must contain outputs but no inputs.
But also, due to fees being variable, it may be useful to have some kind of "remainder" option so that you can say, for example, "give 1 btc apiece to the first 3 addresses, and give the remainder to the last address"
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2869219420)
Can a PSBT be constructed with only outputs, no inputs? If so, then maybe `generateblock` could accept a psbt or a string as an argument. If it is a psbt, it must contain outputs but no inputs.
But also, due to fees being variable, it may be useful to have some kind of "remainder" option so that you can say, for example, "give 1 btc apiece to the first 3 addresses, and give the remainder to the last address"
💬 pokrovskyy commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2869297376)
Meta-protocols builders will find ways to store data on-chain regardless of the decision here. Currently it's in witness data. So this discussion does not solve any raised questions on storing data and its legality etc. However, enabling data in OP_RETURN does offer a number of benefits. Eg.:
- future meta-protocols may choose OP_RETURN as the storage option for their data (instead of witness)
- this will decrease the data inflow into witness
- consequently, this will decrease the growth rate
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2869297376)
Meta-protocols builders will find ways to store data on-chain regardless of the decision here. Currently it's in witness data. So this discussion does not solve any raised questions on storing data and its legality etc. However, enabling data in OP_RETURN does offer a number of benefits. Eg.:
- future meta-protocols may choose OP_RETURN as the storage option for their data (instead of witness)
- this will decrease the data inflow into witness
- consequently, this will decrease the growth rate
...
💬 va7wv commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2869377798)
> @1ma As was discussed in the mailing list discussion, entities are using unspendable outputs in liu of OP_Return outputs. Precisely because of the size limit. This increases the UTXO set size unnecessarily, a harmful effect of having the arbitrary OP_Return output limitations. Your analysis does not take that kind of issue into account.
>
> Rather than do a stream of incremental increases, wasting dev bandwidth, this pull-req simply removes those arbitrary limits.
>
> Anyway, this kind o
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2869377798)
> @1ma As was discussed in the mailing list discussion, entities are using unspendable outputs in liu of OP_Return outputs. Precisely because of the size limit. This increases the UTXO set size unnecessarily, a harmful effect of having the arbitrary OP_Return output limitations. Your analysis does not take that kind of issue into account.
>
> Rather than do a stream of incremental increases, wasting dev bandwidth, this pull-req simply removes those arbitrary limits.
>
> Anyway, this kind o
...
🤔 shahsb reviewed a pull request: "threading: drop CSemaphore in favor of c++20 std::counting_semaphore"
(https://github.com/bitcoin/bitcoin/pull/32466#pullrequestreview-2831290143)
LGTM.. Thanks for making the changes!
(https://github.com/bitcoin/bitcoin/pull/32466#pullrequestreview-2831290143)
LGTM.. Thanks for making the changes!
💬 polespinasa commented on pull request "rpc:generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2869613736)
> Can a PSBT be constructed with only outputs, no inputs?
I'm not sure using a PSBT is the best approach. If we check the [normal workflow](https://github.com/bitcoin/bitcoin/blob/master/doc/psbt.md#overall-workflow) for a PSBT we see it is intended to:
1. Create the PSBT with inputs and outputs but no metadata
2. Updated with the data needed to spend the UTXOs in the inputs
3. Signed
4. Finalized
IMO using PSBTs here does not make sense, we don't need any of that, we don't need to sha
...
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2869613736)
> Can a PSBT be constructed with only outputs, no inputs?
I'm not sure using a PSBT is the best approach. If we check the [normal workflow](https://github.com/bitcoin/bitcoin/blob/master/doc/psbt.md#overall-workflow) for a PSBT we see it is intended to:
1. Create the PSBT with inputs and outputs but no metadata
2. Updated with the data needed to spend the UTXOs in the inputs
3. Signed
4. Finalized
IMO using PSBTs here does not make sense, we don't need any of that, we don't need to sha
...
💬 supertestnet commented on pull request "rpc:generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2869615508)
> Do you have any idea on how that could look like?
Here is how the syntax looks for the `sendmany` command which I am using as inspiration:
`bitcoin-cli sendmany "" "{\"bc1q09vm5lfy0j5reeulh4x5752q25uqqvz34hufdl\":0.01,\"bc1q02ad21edsxd23d32dfgqqsz4vv4nmtfzuklhy3\":0.02}"`
So maybe we could do something similar:
`bitcoin-cli generatetomany "{\"bc1q09vm5lfy0j5reeulh4x5752q25uqqvz34hufdl\":0.01,\"bc1q02ad21edsxd23d32dfgqqsz4vv4nmtfzuklhy3\":0.01,\"bc1qmaf2fjnklfaffz385329cxcr267renn09
...
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2869615508)
> Do you have any idea on how that could look like?
Here is how the syntax looks for the `sendmany` command which I am using as inspiration:
`bitcoin-cli sendmany "" "{\"bc1q09vm5lfy0j5reeulh4x5752q25uqqvz34hufdl\":0.01,\"bc1q02ad21edsxd23d32dfgqqsz4vv4nmtfzuklhy3\":0.02}"`
So maybe we could do something similar:
`bitcoin-cli generatetomany "{\"bc1q09vm5lfy0j5reeulh4x5752q25uqqvz34hufdl\":0.01,\"bc1q02ad21edsxd23d32dfgqqsz4vv4nmtfzuklhy3\":0.01,\"bc1qmaf2fjnklfaffz385329cxcr267renn09
...
💬 polespinasa commented on pull request "rpc:generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2869620363)
> note the remainder in the last btc address
what if you want to split the reminder between multiple addresses?
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2869620363)
> note the remainder in the last btc address
what if you want to split the reminder between multiple addresses?
💬 pithosian commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2083464617)
For additional context, in `mempool_args.cpp::apply_args_man_options`, we have this check:
```cpp
if (argsman.GetBoolArg("-datacarrier", DEFAULT_ACCEPT_DATACARRIER)) {
mempool_opts.max_datacarrier_bytes = argsman.GetIntArg("-datacarriersize", MAX_OP_RETURN_RELAY);
} else {
mempool_opts.max_datacarrier_bytes = std::nullopt;
}
```
`GetIntArg` can't take `nullopt` as a default (it wants an integer), so the true path has to set to `MAX_OP_RETURN_RELAY` as the default, rather than s
...
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2083464617)
For additional context, in `mempool_args.cpp::apply_args_man_options`, we have this check:
```cpp
if (argsman.GetBoolArg("-datacarrier", DEFAULT_ACCEPT_DATACARRIER)) {
mempool_opts.max_datacarrier_bytes = argsman.GetIntArg("-datacarriersize", MAX_OP_RETURN_RELAY);
} else {
mempool_opts.max_datacarrier_bytes = std::nullopt;
}
```
`GetIntArg` can't take `nullopt` as a default (it wants an integer), so the true path has to set to `MAX_OP_RETURN_RELAY` as the default, rather than s
...
💬 pithosian commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2869652723)
Code ACK, conceptual NACK purely on option deprecation (comment [here](https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2868741871) and in the mailing list; the justification for deprecating/removing these options appears faulty to me).
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2869652723)
Code ACK, conceptual NACK purely on option deprecation (comment [here](https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2868741871) and in the mailing list; the justification for deprecating/removing these options appears faulty to me).
💬 brunoerg commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2083467901)
Why does it need `logging.h`?
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2083467901)
Why does it need `logging.h`?