💬 TheCharlatan commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#issuecomment-3466336991)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33738#issuecomment-3466336991)
Concept ACK
💬 maflcko commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2476602340)
in theory this seems racy, when the logging is toggled in another thread, but this is probably good enough to ignore.
Though, on a different note, I wonder if there are other places that could benefit from CTransaction caching. Two full hashes are cached, so caching the size should be cheap in both time and space?
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2476602340)
in theory this seems racy, when the logging is toggled in another thread, but this is probably good enough to ignore.
Though, on a different note, I wonder if there are other places that could benefit from CTransaction caching. Two full hashes are cached, so caching the size should be cheap in both time and space?
🤔 maflcko reviewed a pull request: "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled"
(https://github.com/bitcoin/bitcoin/pull/33738#pullrequestreview-3397666497)
is there a benchmark that detects this?
(https://github.com/bitcoin/bitcoin/pull/33738#pullrequestreview-3397666497)
is there a benchmark that detects this?
💬 l0rinc commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2476611373)
the hash is actually eagerly calculated as well - which should also be hidden behind a similar condition, was hoping someone will bring it up :)
----
there aren't any benchmarks for this but a ton of tests failed for the throw check above, so at lest it has coverage.
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2476611373)
the hash is actually eagerly calculated as well - which should also be hidden behind a similar condition, was hoping someone will bring it up :)
----
there aren't any benchmarks for this but a ton of tests failed for the throw check above, so at lest it has coverage.
👋 l0rinc's pull request is ready for review: "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled"
(https://github.com/bitcoin/bitcoin/pull/33738)
(https://github.com/bitcoin/bitcoin/pull/33738)
💬 maflcko commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3466385489)
I don't think you did. The conflicts are still there and the DrahtBot summary is correct. You can also try it locally:
```
# git checkout b97db6f06185a1d63828f958b33a7e6780aee73c && git merge 1ae9fd3f7a4fddc786e9921e7fc2af41ab96bf9a
HEAD is now at b97db6f061 refactor: unify container presence checks - non-trivial counts
Auto-merging src/init.cpp
Auto-merging src/net_processing.cpp
Auto-merging src/node/interfaces.cpp
Auto-merging src/node/miner.cpp
Auto-merging src/rpc/mempool.cpp
Au
...
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3466385489)
I don't think you did. The conflicts are still there and the DrahtBot summary is correct. You can also try it locally:
```
# git checkout b97db6f06185a1d63828f958b33a7e6780aee73c && git merge 1ae9fd3f7a4fddc786e9921e7fc2af41ab96bf9a
HEAD is now at b97db6f061 refactor: unify container presence checks - non-trivial counts
Auto-merging src/init.cpp
Auto-merging src/net_processing.cpp
Auto-merging src/node/interfaces.cpp
Auto-merging src/node/miner.cpp
Auto-merging src/rpc/mempool.cpp
Au
...
💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2476660588)
It could be, but this is safe as-is, because the `[]` operator defined on the Chain does the check for us.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2476660588)
It could be, but this is safe as-is, because the `[]` operator defined on the Chain does the check for us.
💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2476663196)
Unlike many of the other hashes, we do cache the transaction hashes internally. Since a lookup by txid is a fairly common operation, I think it is fair that we surface this optimization to developers using the API as well.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2476663196)
Unlike many of the other hashes, we do cache the transaction hashes internally. Since a lookup by txid is a fairly common operation, I think it is fair that we surface this optimization to developers using the API as well.
💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3466427632)
> Are the assert statements in src/kernel/bitcoinkernel.cpp really necessary, or could they be replaced with runtime validation?
Replacing them with validation and returning an error works too. However that does not solve the problem of handling assertions within the code, since we have many places within validation that assert on an expected invariant. They are supposed to guard against mistakes from the programmer. Have you tried stubbing out the assertion code / header in a similar way to
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3466427632)
> Are the assert statements in src/kernel/bitcoinkernel.cpp really necessary, or could they be replaced with runtime validation?
Replacing them with validation and returning an error works too. However that does not solve the problem of handling assertions within the code, since we have many places within validation that assert on an expected invariant. They are supposed to guard against mistakes from the programmer. Have you tried stubbing out the assertion code / header in a similar way to
...
🤔 maflcko reviewed a pull request: "rpc: Optionally print feerates in sat/vb"
(https://github.com/bitcoin/bitcoin/pull/33741#pullrequestreview-3397729983)
I guess it is a bit verbose to have each RPC annotated with a type optional arg, but there probably isn't an easier solution.
There could be a global option to toggle the default, to improve the UX.
Though, please don't use floating point types. For monetary values, a fixed point type should be used.
(https://github.com/bitcoin/bitcoin/pull/33741#pullrequestreview-3397729983)
I guess it is a bit verbose to have each RPC annotated with a type optional arg, but there probably isn't an easier solution.
There could be a global option to toggle the default, to improve the UX.
Though, please don't use floating point types. For monetary values, a fixed point type should be used.
💬 maflcko commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2476651745)
Please don't encode default values in how the code is written. This makes it impossible to change the default values without rewriting the code logic.
Also, could use `self.(Maybe)Arg<bool>` instead?
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2476651745)
Please don't encode default values in how the code is written. This makes it impossible to change the default values without rewriting the code logic.
Also, could use `self.(Maybe)Arg<bool>` instead?
💬 maflcko commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2476689034)
this is wrong, you can't use `double` to denote monetary amounts. `double` is a floating point type, which can not represent decimal values accurately.
For example, if `GetFeePerK()` returns `CAmount{665714}`, the Univalue will be `665.7140000000001`, which is confusing at best, but also wrong.
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2476689034)
this is wrong, you can't use `double` to denote monetary amounts. `double` is a floating point type, which can not represent decimal values accurately.
For example, if `GetFeePerK()` returns `CAmount{665714}`, the Univalue will be `665.7140000000001`, which is confusing at best, but also wrong.
💬 l0rinc commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3466597361)
Thanks, looks like a missed a few manual undos.
Reverted the 3 remaining files that seem related to the cluster mempool work and verified that it merges cleanly with https://github.com/bitcoin/bitcoin/pull/33629 and https://github.com/bitcoin/bitcoin/pull/33591
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3466597361)
Thanks, looks like a missed a few manual undos.
Reverted the 3 remaining files that seem related to the cluster mempool work and verified that it merges cleanly with https://github.com/bitcoin/bitcoin/pull/33629 and https://github.com/bitcoin/bitcoin/pull/33591
💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476860710)
Good question. I wanted to keep it limited to member fields (and their constructors) only. Otherwise, there'd be way too many function calls to touch. Happy to do those in a follow-up. Also, happy to drop the `src/node/blockstorage.cpp` changes from this commit, to keep it more focussed on just member fields.
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476860710)
Good question. I wanted to keep it limited to member fields (and their constructors) only. Otherwise, there'd be way too many function calls to touch. Happy to do those in a follow-up. Also, happy to drop the `src/node/blockstorage.cpp` changes from this commit, to keep it more focussed on just member fields.
💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476885390)
> This value will be returned and converted to a `size_t` again
Correct, but this is safe, because the range was checked in the `if` below.
Generally, the idea is that 32-bits is just not really enough to do size accounting (even on 32-bit architectures), because some code paths may accumulate sizes over time, or multiply them with a factor, all of which may or may not overflow the limited 32-bit value.
So using 64-bit arithmetic is overall safer, and more future proof, because current
...
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476885390)
> This value will be returned and converted to a `size_t` again
Correct, but this is safe, because the range was checked in the `if` below.
Generally, the idea is that 32-bits is just not really enough to do size accounting (even on 32-bit architectures), because some code paths may accumulate sizes over time, or multiply them with a factor, all of which may or may not overflow the limited 32-bit value.
So using 64-bit arithmetic is overall safer, and more future proof, because current
...
💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476889493)
yes, but see above
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476889493)
yes, but see above
💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476889544)
Yeah, I can go that way, but I think using 64-bit by default and explicitly casting after the range has been properly checked seems safer, see also https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476885390
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476889544)
Yeah, I can go that way, but I think using 64-bit by default and explicitly casting after the range has been properly checked seems safer, see also https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476885390
💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476889665)
Correct, but I wanted to keep this commit limited to just member fields, see https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476860710. Happy to revert this hunk and leave if for a follow-up.
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476889665)
Correct, but I wanted to keep this commit limited to just member fields, see https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476860710. Happy to revert this hunk and leave if for a follow-up.
💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476889813)
I wanted to keep this one focussed on the kernel. Wallet changes can be done in a follow-up, if that is fine?
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476889813)
I wanted to keep this one focussed on the kernel. Wallet changes can be done in a follow-up, if that is fine?
💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476890208)
(see above)
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476890208)
(see above)