💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908477798)
Did you hard-code the -1 literal? I don't think that holds for a variable `mib` at runtime?
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908477798)
Did you hard-code the -1 literal? I don't think that holds for a variable `mib` at runtime?
💬 l0rinc commented on pull request "RPC: Fix invalid txid in `gettransaction` example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2579657065)
> An old txid will only work with txindex enabled, though
This is also true for the new one - please refer to the example in the PR's description.
@luke-jr, @jonatack, do you have any concerns about the current approach?
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2579657065)
> An old txid will only work with txindex enabled, though
This is also true for the new one - please refer to the example in the PR's description.
@luke-jr, @jonatack, do you have any concerns about the current approach?
💬 theStack commented on pull request "test: raise explicit error if any of the needed release binaries is missing":
(https://github.com/bitcoin/bitcoin/pull/31462#discussion_r1908483101)
Will leave the PR as-is, but happy to review a follow-up PR which refines test/README.md about previous releases and/or adds improvements to the previous release download script itself.
> Last thing is that "At least one release" is not entirely correct, we need the last previous to the current, if you delete the v.28.0 from the releases folder (as specified in the https://github.com/bitcoin/bitcoin/pull/31462#issue-2730936530 of the PR) and you still have v25.0 and/ or v24.0.1, you will get
...
(https://github.com/bitcoin/bitcoin/pull/31462#discussion_r1908483101)
Will leave the PR as-is, but happy to review a follow-up PR which refines test/README.md about previous releases and/or adds improvements to the previous release download script itself.
> Last thing is that "At least one release" is not entirely correct, we need the last previous to the current, if you delete the v.28.0 from the releases folder (as specified in the https://github.com/bitcoin/bitcoin/pull/31462#issue-2730936530 of the PR) and you still have v25.0 and/ or v24.0.1, you will get
...
💬 l0rinc commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2579687987)
> And doing that all over would be a recipe for never-ending churn
Isn't that what we're doing regularly though? Endlessly making the legacy codebase a tiny bit more readable, a tiny bit more modern, a tiny bit more maintainable and consistent?
> I'd rather not be doing random opinionated changes like this.
Previously, I've applied the following changes (as mentioned in the commit message), but I agree that a few of these could be reverted as being indeed opinionated:
- Replaced `unsig
...
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2579687987)
> And doing that all over would be a recipe for never-ending churn
Isn't that what we're doing regularly though? Endlessly making the legacy codebase a tiny bit more readable, a tiny bit more modern, a tiny bit more maintainable and consistent?
> I'd rather not be doing random opinionated changes like this.
Previously, I've applied the following changes (as mentioned in the commit message), but I agree that a few of these could be reverted as being indeed opinionated:
- Replaced `unsig
...
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1908512682)
Good point, maybe add:
```
The previous `-blockmaxweight` default was `3,996,000`. The block template construction code added an additional `4,000` padding. The new default `-maxcoinbaseweight` value was chosen to maintain that behaviour.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1908512682)
Good point, maybe add:
```
The previous `-blockmaxweight` default was `3,996,000`. The block template construction code added an additional `4,000` padding. The new default `-maxcoinbaseweight` value was chosen to maintain that behaviour.
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1908514307)
Correct.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1908514307)
Correct.
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908520794)
Yes, I hard-coded the literal into the `static_assert` above. What part of integer arithmetic and casting changes when done in a runtime context?
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908520794)
Yes, I hard-coded the literal into the `static_assert` above. What part of integer arithmetic and casting changes when done in a runtime context?
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1908522924)
Current `ClampOptions` has a comment that specifically permits `-maxcoinbaseweight` to exceed `-blockmaxweight`. It just means that all blocks will be empty. This seems like an unrealistic scenario anyway.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1908522924)
Current `ClampOptions` has a comment that specifically permits `-maxcoinbaseweight` to exceed `-blockmaxweight`. It just means that all blocks will be empty. This seems like an unrealistic scenario anyway.
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908525582)
Even if the assert is correct, maybe the fact that this thread exists is justification enough for replacing it with something closer to what it was originally. Like https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907618229
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908525582)
Even if the assert is correct, maybe the fact that this thread exists is justification enough for replacing it with something closer to what it was originally. Like https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907618229
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1908533084)
I agree we should not change the `weight` and `sigops` fields in `getblocktemplate` and `getmininginfo`.
We could either adjust the value in the RPC code, or as @luke-jr suggests restore the original behavior here.
Additionally we should clarify the help text for these RPC methods that they include the coinbase reserved weight.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1908533084)
I agree we should not change the `weight` and `sigops` fields in `getblocktemplate` and `getmininginfo`.
We could either adjust the value in the RPC code, or as @luke-jr suggests restore the original behavior here.
Additionally we should clarify the help text for these RPC methods that they include the coinbase reserved weight.
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1908538059)
Perhaps `-coinbaseweightreservation` is a better name.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1908538059)
Perhaps `-coinbaseweightreservation` is a better name.
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1908542626)
Seems reasonable to keep and just make it `DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT};`
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1908542626)
Seems reasonable to keep and just make it `DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT};`
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1908547490)
Sorry, I think @luke-jr is right and we shouldn't change this: https://github.com/bitcoin/bitcoin/pull/31384/files#r1908533084
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1908547490)
Sorry, I think @luke-jr is right and we shouldn't change this: https://github.com/bitcoin/bitcoin/pull/31384/files#r1908533084
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2579834709)
> I also have one last lingering question which is: when does the miner actually construct their coinbase transaction and is that ever compared to the reserved size?
This is entirely up to the pool software. They could call `getblocktemplate` in `proposal` mode (or `checkblock` if #31564 lands).
I think https://github.com/bitcoin/bitcoin/pull/31384/files#r1908533084 should be addressed before merging (and so might as well address the other comments now instead of a followup).
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2579834709)
> I also have one last lingering question which is: when does the miner actually construct their coinbase transaction and is that ever compared to the reserved size?
This is entirely up to the pool software. They could call `getblocktemplate` in `proposal` mode (or `checkblock` if #31564 lands).
I think https://github.com/bitcoin/bitcoin/pull/31384/files#r1908533084 should be addressed before merging (and so might as well address the other comments now instead of a followup).
📝 TheCharlatan converted_to_draft a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483)
Carrying non-kernel related fields in the cache sizes for the indexes is confusing for kernel library users. The cache sizes are set currently with magic numbers in bitcoin-chainstate. The comments for the cache size calculations are not completely clear. The constants for the cache sizes are also currently in `txdb.h`, which is not an ideal place for holding all cache size related constants.
Solve these things by moving the kernel-specific cache size fields to their own struct and moving the
...
(https://github.com/bitcoin/bitcoin/pull/31483)
Carrying non-kernel related fields in the cache sizes for the indexes is confusing for kernel library users. The cache sizes are set currently with magic numbers in bitcoin-chainstate. The comments for the cache size calculations are not completely clear. The constants for the cache sizes are also currently in `txdb.h`, which is not an ideal place for holding all cache size related constants.
Solve these things by moving the kernel-specific cache size fields to their own struct and moving the
...
✅ Sjors closed a pull request: "getblocktemplate improvements for segwit and sigops"
(https://github.com/bitcoin/bitcoin/pull/27433)
(https://github.com/bitcoin/bitcoin/pull/27433)
💬 Sjors commented on pull request "getblocktemplate improvements for segwit and sigops":
(https://github.com/bitcoin/bitcoin/pull/27433#issuecomment-2579855238)
Closing this in favor of two seperate PRs for 9fd56f0ae4b301da38d9e12c57b2ca34941a8d1c and 8a2509e21abfccf64d375b0700034a478ab8aac8. I left 4facb94a481ca121a9117e0992799371bb429da1 as a suggestion for https://github.com/bitcoin/bitcoin/pull/31384/#issuecomment-2579834709.
(https://github.com/bitcoin/bitcoin/pull/27433#issuecomment-2579855238)
Closing this in favor of two seperate PRs for 9fd56f0ae4b301da38d9e12c57b2ca34941a8d1c and 8a2509e21abfccf64d375b0700034a478ab8aac8. I left 4facb94a481ca121a9117e0992799371bb429da1 as a suggestion for https://github.com/bitcoin/bitcoin/pull/31384/#issuecomment-2579834709.
📝 Sjors opened a pull request: "doc: warn that CheckBlock() underestimates sigops"
(https://github.com/bitcoin/bitcoin/pull/31624)
Counting sigops in the witness requires context that CheckBlock() does not have, so it only counts sigops for non-segwit transactions.
It's useful to document, but it should not be a problem.
(https://github.com/bitcoin/bitcoin/pull/31624)
Counting sigops in the witness requires context that CheckBlock() does not have, so it only counts sigops for non-segwit transactions.
It's useful to document, but it should not be a problem.
💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908583360)
Hmm, you raise a good point. I see two alternatives:
1) The low-code solution: when values get problematic (which is pretty infrequent, and constant), write them as one of follows:
```c++
static constexpr size_t DEFAULT_KERNEL_CACHE{3145728000};
static_assert(DEFAULT_KERNEL_CACHE >> 20 == 3000);
```
or
```c++
static constexpr size_t DEFAULT_KERNEL_CACHE{size_t{3000} << 20};
static_assert(DEFAULT_KERNEL_CACHE >> 20 == 3000);
```
2) instead of adding a `MiBToBytes` function,
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908583360)
Hmm, you raise a good point. I see two alternatives:
1) The low-code solution: when values get problematic (which is pretty infrequent, and constant), write them as one of follows:
```c++
static constexpr size_t DEFAULT_KERNEL_CACHE{3145728000};
static_assert(DEFAULT_KERNEL_CACHE >> 20 == 3000);
```
or
```c++
static constexpr size_t DEFAULT_KERNEL_CACHE{size_t{3000} << 20};
static_assert(DEFAULT_KERNEL_CACHE >> 20 == 3000);
```
2) instead of adding a `MiBToBytes` function,
...
💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908587275)
> I don't think I see a new test case added in the diff. Is it missing?
Snap, they weren't included in my `git diff` because they were added in a new file, and I didn't notice, sorry. I see @TheCharlatan already added a test case in his latest push.
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908587275)
> I don't think I see a new test case added in the diff. Is it missing?
Snap, they weren't included in my `git diff` because they were added in a new file, and I didn't notice, sorry. I see @TheCharlatan already added a test case in his latest push.