💬 ryanofsky commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2614537274)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2608405722
> I'm guessing the reason for the existence of a separate `SuccessHolder` type is in order to specialize away a minimal subset of functionality in `SuccessHolder<void, ...>`. If that's part of the reason, it could be admitted in the comment block for the main `SuccessHolder` template?
Yes exactly. Added a comment mentioning the void type to make this clearer.
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2614537274)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2608405722
> I'm guessing the reason for the existence of a separate `SuccessHolder` type is in order to specialize away a minimal subset of functionality in `SuccessHolder<void, ...>`. If that's part of the reason, it could be admitted in the comment block for the main `SuccessHolder` template?
Yes exactly. Added a comment mentioning the void type to make this clearer.
💬 ryanofsky commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2614454078)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2593959586
> Isn't it closer to: `tuple<optional<SuccessType>, unique_ptr<tuple<FailureType, MessagesType>>>`
The current comment here says "Logically, a result object is equivalent to `tuple<variant<SuccessType, ErrorType>, MessagesType>`".
This is just trying to get across the idea that a `Result` holds a success value or a failure value, plus some messages.
IMO, adding unique_ptr to this description would make it more
...
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2614454078)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2593959586
> Isn't it closer to: `tuple<optional<SuccessType>, unique_ptr<tuple<FailureType, MessagesType>>>`
The current comment here says "Logically, a result object is equivalent to `tuple<variant<SuccessType, ErrorType>, MessagesType>`".
This is just trying to get across the idea that a `Result` holds a success value or a failure value, plus some messages.
IMO, adding unique_ptr to this description would make it more
...
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3646978232)
Many thanks!
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3646978232)
Many thanks!
📝 marcofleon opened a pull request: "fuzz: Fix bugs in `clusterlin_postlinearize_tree` target"
(https://github.com/bitcoin/bitcoin/pull/34061)
Addresses two issues in the `clusterlin_postlinearize_tree` target:
1. The loop iteration while creating tree dependency graphs was incorrect.
2. We were accidentally passing in `post_linearization` to `PostLinearize` instead of the copy we just made, resulting in an ineffective check.
(https://github.com/bitcoin/bitcoin/pull/34061)
Addresses two issues in the `clusterlin_postlinearize_tree` target:
1. The loop iteration while creating tree dependency graphs was incorrect.
2. We were accidentally passing in `post_linearization` to `PostLinearize` instead of the copy we just made, resulting in an ineffective check.
💬 fanquake commented on pull request "fuzz: Fix bugs in `clusterlin_postlinearize_tree` target":
(https://github.com/bitcoin/bitcoin/pull/34061#issuecomment-3647001408)
cc @sipa @instagibbs @glozow
(https://github.com/bitcoin/bitcoin/pull/34061#issuecomment-3647001408)
cc @sipa @instagibbs @glozow
💬 maflcko commented on pull request "net: Waste less time in socket handling":
(https://github.com/bitcoin/bitcoin/pull/34025#discussion_r2614627662)
> because time_point doesn't support being an atomic.
Duration does not either. I think it is fine to type `.load()` or `.store()`, where needed.
I guess there is no rush here, and this can be done, when all other places (non-atomic) are switched.
(https://github.com/bitcoin/bitcoin/pull/34025#discussion_r2614627662)
> because time_point doesn't support being an atomic.
Duration does not either. I think it is fine to type `.load()` or `.store()`, where needed.
I guess there is no rush here, and this can be done, when all other places (non-atomic) are switched.
💬 sipa commented on pull request "fuzz: Fix bugs in `clusterlin_postlinearize_tree` target":
(https://github.com/bitcoin/bitcoin/pull/34061#issuecomment-3647032758)
ACK a70a14a3f4f4d66025302c00eb4ff8108835cc5d
(https://github.com/bitcoin/bitcoin/pull/34061#issuecomment-3647032758)
ACK a70a14a3f4f4d66025302c00eb4ff8108835cc5d
⚠️ stickies-v opened an issue: "RFC: separate kernel logging infrastructure"
(https://github.com/bitcoin/bitcoin/issues/34062)
tl;dr: kernel logging is cumbersome. I propose we separate it so we can enable structured logging and a much simplified logging interface without overcomplicating node logging.
## Motivation
The `bitcoinkernel` library (#27587) [exposes](https://github.com/bitcoin/bitcoin/blob/b26762bdcb941096ccc6f53c3fb5a7786ad735e7/src/kernel/bitcoinkernel.h#L698-L778) functionality to interface with the kernel logging. This includes registering callbacks for log statements, level/category filtering, string
...
(https://github.com/bitcoin/bitcoin/issues/34062)
tl;dr: kernel logging is cumbersome. I propose we separate it so we can enable structured logging and a much simplified logging interface without overcomplicating node logging.
## Motivation
The `bitcoinkernel` library (#27587) [exposes](https://github.com/bitcoin/bitcoin/blob/b26762bdcb941096ccc6f53c3fb5a7786ad735e7/src/kernel/bitcoinkernel.h#L698-L778) functionality to interface with the kernel logging. This includes registering callbacks for log statements, level/category filtering, string
...
📝 billymcbip converted_to_draft a pull request: "test: Improve STRICTENC/DERSIG unit tests in script_tests.json"
(https://github.com/bitcoin/bitcoin/pull/33973)
Use the `DERSIG` flag instead of the `STRICTENC` flag for signature encoding tests. `STRICTENC` rules are a superset of `DERSIG` rules, and only `DERSIG` is a consensus flag. Also improve test descriptions and add two error cases to the `CHECKMULTISIG NOT` section to make it easier to understand how these tests work.
Tests pass on `6224b272ac291afd3af89f91e1deaccf233718bf`:
```
cmake -B build -DENABLE_WALLET=OFF
cmake --build build --parallel 8
ctest --test-dir build --parallel 8
```
``
...
(https://github.com/bitcoin/bitcoin/pull/33973)
Use the `DERSIG` flag instead of the `STRICTENC` flag for signature encoding tests. `STRICTENC` rules are a superset of `DERSIG` rules, and only `DERSIG` is a consensus flag. Also improve test descriptions and add two error cases to the `CHECKMULTISIG NOT` section to make it easier to understand how these tests work.
Tests pass on `6224b272ac291afd3af89f91e1deaccf233718bf`:
```
cmake -B build -DENABLE_WALLET=OFF
cmake --build build --parallel 8
ctest --test-dir build --parallel 8
```
``
...
💬 ryanofsky commented on issue "RFC: separate kernel logging infrastructure":
(https://github.com/bitcoin/bitcoin/issues/34062#issuecomment-3647259207)
Nice work. The new interface proposed here and implemented in your branch seem much better than the current interface, and well thought out. I feel like we could pretty easily wire it into the existing Logger class without introducing a new class and new macros though, and without needing to change validation & util code.
I wonder if there may be difficulties doing that I'm not aware of, or if it just seemed easier to start with a clean state in the current branch.
I do think it would preferab
...
(https://github.com/bitcoin/bitcoin/issues/34062#issuecomment-3647259207)
Nice work. The new interface proposed here and implemented in your branch seem much better than the current interface, and well thought out. I feel like we could pretty easily wire it into the existing Logger class without introducing a new class and new macros though, and without needing to change validation & util code.
I wonder if there may be difficulties doing that I'm not aware of, or if it just seemed easier to start with a clean state in the current branch.
I do think it would preferab
...
📝 rustaceanrob opened a pull request: "Make `transaction_indentifier` hex string constructor evaluated at comptime"
(https://github.com/bitcoin/bitcoin/pull/34063)
Suggested by @l0rinc as a comment in #34004
There are tests that utilize `FromHex` that will only fail during runtime if malformed. Adds a compile time constructor that can be caught by LSPs
(https://github.com/bitcoin/bitcoin/pull/34063)
Suggested by @l0rinc as a comment in #34004
There are tests that utilize `FromHex` that will only fail during runtime if malformed. Adds a compile time constructor that can be caught by LSPs
💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3647326460)
I have rewritten `SpanningForestState::UpdateChunk` to no longer traverse the chunk by walking dependencies, but by looping over all dependencies of the chunk directly, and using the `top_setinfo` to figure out which ones to update. It's a few percent faster, simpler, and additionally allowed dropping the tracking of parent dependencies of a transaction entirely.
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3647326460)
I have rewritten `SpanningForestState::UpdateChunk` to no longer traverse the chunk by walking dependencies, but by looping over all dependencies of the chunk directly, and using the `top_setinfo` to figure out which ones to update. It's a few percent faster, simpler, and additionally allowed dropping the tracking of parent dependencies of a transaction entirely.
👍 instagibbs approved a pull request: "fuzz: Fix bugs in `clusterlin_postlinearize_tree` target"
(https://github.com/bitcoin/bitcoin/pull/34061#pullrequestreview-3572724599)
ACK a70a14a3f4f4d66025302c00eb4ff8108835cc5d
(https://github.com/bitcoin/bitcoin/pull/34061#pullrequestreview-3572724599)
ACK a70a14a3f4f4d66025302c00eb4ff8108835cc5d
💬 davidgumberg commented on pull request "doc: guix: Troubleshooting zdiff3 issue and uninstalling.":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2614933849)
The mistake I made was that `or running...` should be inside of the parentheses. The items in the parentheses don't "need" to be parallel with "Uninstall" even if you respect the parallel structure device as a rule (which it isn't.)
I moved the guix install script instructions inside the parentheses.
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2614933849)
The mistake I made was that `or running...` should be inside of the parentheses. The items in the parentheses don't "need" to be parallel with "Uninstall" even if you respect the parallel structure device as a rule (which it isn't.)
I moved the guix install script instructions inside the parentheses.
💬 andrewtoth commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#issuecomment-3647464529)
@sedited @l0rinc thank you both for your reviews! I rebased and updated with your suggestions.
(https://github.com/bitcoin/bitcoin/pull/33018#issuecomment-3647464529)
@sedited @l0rinc thank you both for your reviews! I rebased and updated with your suggestions.
🤔 l0rinc requested changes to a pull request: "test: Add test on skip heights in CBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/33661#pullrequestreview-3572005492)
I might have gone a bit overboard, but I have tried explaining every change that I suggested in context.
For convenience, here's the full local diff I did during review (this is the latest state, some of the suggestions may reflect a previous one):
```C++
BOOST_AUTO_TEST_CASE(get_skip_height_test)
{
// Even heights: clear the lowest set bit.
BOOST_CHECK_EQUAL(GetSkipHeight(0b00010010),
0b00010000);
BOOST_CHECK_EQUAL(GetSkipHeight(0b00100010),
...
(https://github.com/bitcoin/bitcoin/pull/33661#pullrequestreview-3572005492)
I might have gone a bit overboard, but I have tried explaining every change that I suggested in context.
For convenience, here's the full local diff I did during review (this is the latest state, some of the suggestions may reflect a previous one):
```C++
BOOST_AUTO_TEST_CASE(get_skip_height_test)
{
// Even heights: clear the lowest set bit.
BOOST_CHECK_EQUAL(GetSkipHeight(0b00010010),
0b00010000);
BOOST_CHECK_EQUAL(GetSkipHeight(0b00100010),
...
💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614369333)
Could unify these to `BOOST_REQUIRE` instead of `assert`
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614369333)
Could unify these to `BOOST_REQUIRE` instead of `assert`
💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614388593)
can you use brace init consistently to avoid narrowing? And corecheck suggests const reference - though we can likely inline it instead...
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614388593)
can you use brace init consistently to avoid narrowing? And corecheck suggests const reference - though we can likely inline it instead...
💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614370366)
return value isn't needed, could be simplified to:
```suggestion
--heightWalk;
```
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614370366)
return value isn't needed, could be simplified to:
```suggestion
--heightWalk;
```
💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614371127)
```suggestion
BOOST_REQUIRE_GT(delta, 0);
```
And if we keep this, it could go closer to the `delta` declaration
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614371127)
```suggestion
BOOST_REQUIRE_GT(delta, 0);
```
And if we keep this, it could go closer to the `delta` declaration
💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614455685)
why 100 and why 5%? The numbers are already visible in the code, but not sure why - can we have that in the comment instead?
The 5% doesn't seem to hold for non-deterministic random - seems arbitrary.
Neither does the `100` - so my suspicion seems to have been accurate that these aren't real properties of the system, just accidental ones of the sample.
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614455685)
why 100 and why 5%? The numbers are already visible in the code, but not sure why - can we have that in the comment instead?
The 5% doesn't seem to hold for non-deterministic random - seems arbitrary.
Neither does the `100` - so my suspicion seems to have been accurate that these aren't real properties of the system, just accidental ones of the sample.