💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938237527)
Indeed, this could be `MAX_BLOCK_WEIGHT`, but definitely not `max_block_weight`.
The clamping does not prevent someone from creating a 4MB coinbase transaction while otherwise not mining anything by setting `-maxblockweight=0`. As nonsensical as that might be, but dropping `-blockmaxweight` seems better to discuss in a followup.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938237527)
Indeed, this could be `MAX_BLOCK_WEIGHT`, but definitely not `max_block_weight`.
The clamping does not prevent someone from creating a 4MB coinbase transaction while otherwise not mining anything by setting `-maxblockweight=0`. As nonsensical as that might be, but dropping `-blockmaxweight` seems better to discuss in a followup.
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628873833)
@pinheadmz wrote:
> some mining pools to squeeze an extra 4000 WU of transaction data into new blocks
@fjahr wrote:
> Only an extra 2000 WU, right?
Just 2000 extra WU. They could already push the effective reservation to 4000 by setting `blockmaxweight=4000000` and there's a minimum value of `-blockreservedweight=2000`.
@ismaelsadeeq wrote:
> No, the current behavior on master is that we always reserve 8,000 WU, regardless of what blockmaxweight is. This is actually how I disco
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628873833)
@pinheadmz wrote:
> some mining pools to squeeze an extra 4000 WU of transaction data into new blocks
@fjahr wrote:
> Only an extra 2000 WU, right?
Just 2000 extra WU. They could already push the effective reservation to 4000 by setting `blockmaxweight=4000000` and there's a minimum value of `-blockreservedweight=2000`.
@ismaelsadeeq wrote:
> No, the current behavior on master is that we always reserve 8,000 WU, regardless of what blockmaxweight is. This is actually how I disco
...
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628877288)
You're right though I think, the original code clamps like this:
```cpp
static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
{
Assert(options.coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
Assert(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
// Limit weight to between coinbase_max_additional_weight and DEFAULT_BLOCK_MAX_WEIGHT for sanity:
// Coinbase (reserved) outputs can safely exceed -blockmaxweight, but
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628877288)
You're right though I think, the original code clamps like this:
```cpp
static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
{
Assert(options.coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
Assert(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
// Limit weight to between coinbase_max_additional_weight and DEFAULT_BLOCK_MAX_WEIGHT for sanity:
// Coinbase (reserved) outputs can safely exceed -blockmaxweight, but
...
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938239821)
We need to clarify this:
> was `3,996,000`,
to
> was `3,996,000` and any higher value was silently clamped to that.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938239821)
We need to clarify this:
> was `3,996,000`,
to
> was `3,996,000` and any higher value was silently clamped to that.
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938240400)
> can lower `-blockreservedweight` to `4,000` to maintain the previous behaviour.
to
> should be aware that this value previously had no effect, since it was clamped to 3,996,000. Be careful about lowing `-blockreservedweight` to `4,000`.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938240400)
> can lower `-blockreservedweight` to `4,000` to maintain the previous behaviour.
to
> should be aware that this value previously had no effect, since it was clamped to 3,996,000. Be careful about lowing `-blockreservedweight` to `4,000`.
💬 fanquake commented on pull request "depends: Override default build type for `libevent`":
(https://github.com/bitcoin/bitcoin/pull/31661#discussion_r1938240587)
This did break oss-fuzz. See #31770.
(https://github.com/bitcoin/bitcoin/pull/31661#discussion_r1938240587)
This did break oss-fuzz. See #31770.
📝 JoeyVee8666 opened a pull request: "doing whats needed."
(https://github.com/bitcoin/bitcoin/pull/31776)
Cant Automatically merge.<!-
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements
...
(https://github.com/bitcoin/bitcoin/pull/31776)
Cant Automatically merge.<!-
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements
...
✅ fanquake closed a pull request: "doing whats needed."
(https://github.com/bitcoin/bitcoin/pull/31776)
(https://github.com/bitcoin/bitcoin/pull/31776)
📝 fanquake locked a pull request: "doing whats needed."
(https://github.com/bitcoin/bitcoin/pull/31776)
Cant Automatically merge.<!-
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements
...
(https://github.com/bitcoin/bitcoin/pull/31776)
Cant Automatically merge.<!-
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements
...
💬 pinheadmz commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938258005)
Then perhaps something like "Reserve space for the fixed-size block header plus the largest coinbase transaction the mining software may add to the block"
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938258005)
Then perhaps something like "Reserve space for the fixed-size block header plus the largest coinbase transaction the mining software may add to the block"
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2628948969)
Updated fb6e0deee18dff38219c9646d2461a40de03ed66 -> f926d7ef34773b7836e94491a16021288c125b11 ([kernelApi_20](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_20) -> [kernelApi_21](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_21), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_20..kernelApi_21))
* Added symbol exports where appropriate in the header to enable windows support
* Completely replaced the existing bitcoin-chainstate with the new kernel-API-only
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2628948969)
Updated fb6e0deee18dff38219c9646d2461a40de03ed66 -> f926d7ef34773b7836e94491a16021288c125b11 ([kernelApi_20](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_20) -> [kernelApi_21](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_21), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_20..kernelApi_21))
* Added symbol exports where appropriate in the header to enable windows support
* Completely replaced the existing bitcoin-chainstate with the new kernel-API-only
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1938274204)
Ok, added symbol exporting now, so we should have somewhat working windows support.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1938274204)
Ok, added symbol exporting now, so we should have somewhat working windows support.
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938277339)
And maybe
```suggestion
leading to a minimum template size of `3,992,000` in any case.
```
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938277339)
And maybe
```suggestion
leading to a minimum template size of `3,992,000` in any case.
```
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938278047)
And maybe add something like "Be careful about lowing -blockreservedweight to 4,000 and ensure that your coinbase transaction does indeed not exceed 4,000 since there were previously always 8,000 WU available".
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938278047)
And maybe add something like "Be careful about lowing -blockreservedweight to 4,000 and ensure that your coinbase transaction does indeed not exceed 4,000 since there were previously always 8,000 WU available".
💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938267534)
👍 10m seems like a good approximation for the previous value:
> 2025-01-31T09:45:42Z UpdateTip: new best=0000000000000000cc22cc938f92ecc3a3844f23775c4ad4e66404563c8594c8 height=287000 version=0x00000002 log2_work=76.752105 tx=33340892 date='2014-02-2
1T05:18:42Z' progress=0.028794 cache=1245.8MiB(10004443txo)
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938267534)
👍 10m seems like a good approximation for the previous value:
> 2025-01-31T09:45:42Z UpdateTip: new best=0000000000000000cc22cc938f92ecc3a3844f23775c4ad4e66404563c8594c8 height=287000 version=0x00000002 log2_work=76.752105 tx=33340892 date='2014-02-2
1T05:18:42Z' progress=0.028794 cache=1245.8MiB(10004443txo)
🤔 l0rinc requested changes to a pull request: "Use number of dirty cache entries in flush warnings/logs"
(https://github.com/bitcoin/bitcoin/pull/31703#pullrequestreview-2588328910)
I have reviewed it in multiple steps, I have a few remaining questions and concerns, let me know if I can help in any other way.
(https://github.com/bitcoin/bitcoin/pull/31703#pullrequestreview-2588328910)
I have reviewed it in multiple steps, I have a few remaining questions and concerns, let me know if I can help in any other way.
💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938266129)
This now hides `GetDirtyCount` in `CCoinsViewCache` with a `size_t` return value (changed to `size_t&` here), used later as `cache.usage() +=` and `cache.GetDirtyCount() +=`.
To avoid the confusing override and the method return value assignment, please consider making them explicit setters/adders:
```C++
void AddUsage(size_t usage) const { cachedCoinsUsage += usage; }
void AddDirtyCount(size_t count) const { m_dirty_count += count; }
```
```C++
cache.AddUsage(InsertCoinsMapEntry(ca
...
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938266129)
This now hides `GetDirtyCount` in `CCoinsViewCache` with a `size_t` return value (changed to `size_t&` here), used later as `cache.usage() +=` and `cache.GetDirtyCount() +=`.
To avoid the confusing override and the method return value assignment, please consider making them explicit setters/adders:
```C++
void AddUsage(size_t usage) const { cachedCoinsUsage += usage; }
void AddDirtyCount(size_t count) const { m_dirty_count += count; }
```
```C++
cache.AddUsage(InsertCoinsMapEntry(ca
...
💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938275296)
The documentation of `EmplaceCoinInternalDANGER` claims that:
> Emplace a coin into cacheCoins without performing any checks, marking the emplaced coin as dirty.
How come we're updating `cachedCoinsUsage` unconditionally in this method - but `m_dirty_count` only when inserted?
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938275296)
The documentation of `EmplaceCoinInternalDANGER` claims that:
> Emplace a coin into cacheCoins without performing any checks, marking the emplaced coin as dirty.
How come we're updating `cachedCoinsUsage` unconditionally in this method - but `m_dirty_count` only when inserted?
💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938269653)
This change doesn't seem to be covered by `coins_tests` at all.
Can we extend `SimulationTest` to call `stack.back()->EmplaceCoinInternalDANGER` instead of `AddCoin` randomly?
Maybe something like:
```C++
if (COutPoint op(txid, 0); !stack.back()->map().contains(op) && !(coin.IsSpent() || (m_rng.rand32() & 1))) {
stack.back()->EmplaceCoinInternalDANGER(std::move(op), std::move(newcoin));
} else {
stack.back()->AddCoin(std::move(op), std::move(newcoin), /*possible_overwrite=*/(!co
...
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938269653)
This change doesn't seem to be covered by `coins_tests` at all.
Can we extend `SimulationTest` to call `stack.back()->EmplaceCoinInternalDANGER` instead of `AddCoin` randomly?
Maybe something like:
```C++
if (COutPoint op(txid, 0); !stack.back()->map().contains(op) && !(coin.IsSpent() || (m_rng.rand32() & 1))) {
stack.back()->EmplaceCoinInternalDANGER(std::move(op), std::move(newcoin));
} else {
stack.back()->AddCoin(std::move(op), std::move(newcoin), /*possible_overwrite=*/(!co
...
💬 andrewtoth commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938281310)
That's a bug from #28280. It should be done conditionally. I fix it in the first commit of #31132. The only caller of this method will not add coins that already exist in the cache though, so it will always enter the `if (inserted)` block.
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938281310)
That's a bug from #28280. It should be done conditionally. I fix it in the first commit of #31132. The only caller of this method will not add coins that already exist in the cache though, so it will always enter the `if (inserted)` block.