⚠️ 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.
💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614448211)
nit: we could reduce the scope of the iteration-only variables:
```C++
int iteration_count{0};
for (int height_walk{start_height}; height_walk > target_height; ++iteration_count) {
```
nit: `heightWalk` -> `height_walk`
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614448211)
nit: we could reduce the scope of the iteration-only variables:
```C++
int iteration_count{0};
for (int height_walk{start_height}; height_walk > target_height; ++iteration_count) {
```
nit: `heightWalk` -> `height_walk`
💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614428365)
This may be useful during development, but it's basically testing the testing logic itself, isn't it? If you keep them, can you add them after the initializations to have separate init and check sections, something like:
```C++
const int start_height{(chain_size - 1) - ctx.randrange(chain_size / 2)}; // pick a height in the second half of the chain
const int delta{min_distance + ctx.randrange(start_height - min_distance + 1)}; // pick a target height, earlier by at least min_distance
const
...
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614428365)
This may be useful during development, but it's basically testing the testing logic itself, isn't it? If you keep them, can you add them after the initializations to have separate init and check sections, something like:
```C++
const int start_height{(chain_size - 1) - ctx.randrange(chain_size / 2)}; // pick a height in the second half of the chain
const int delta{min_distance + ctx.randrange(start_height - min_distance + 1)}; // pick a target height, earlier by at least min_distance
const
...
💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614408109)
I think you mean that the second and third **set** bits are zerod, right?
```C++
// Even heights: clear the lowest set bit.
...
// Odd heights: clear the 2nd and 3rd set bits.
```
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614408109)
I think you mean that the second and third **set** bits are zerod, right?
```C++
// Even heights: clear the lowest set bit.
...
// Odd heights: clear the 2nd and 3rd set bits.
```
💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614473206)
Given that we've already called `BuildSkip`, do we need to test this directly or can we do it indirectly as well?
```suggestion
const int height_skip{pindexWalk->pskip->nHeight};
const int height_skip_prev{pindexWalk->pprev->pskip->nHeight};
```
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614473206)
Given that we've already called `BuildSkip`, do we need to test this directly or can we do it indirectly as well?
```suggestion
const int height_skip{pindexWalk->pskip->nHeight};
const int height_skip_prev{pindexWalk->pprev->pskip->nHeight};
```
💬 Sjors commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#issuecomment-3647551260)
@cobratbq there's certainly no need to sign commits, but when people do sign, it's nice that we don't break the signature.
(https://github.com/bitcoin/bitcoin/pull/33765#issuecomment-3647551260)
@cobratbq there's certainly no need to sign commits, but when people do sign, it's nice that we don't break the signature.
💬 Sjors commented on pull request "doc: improvements to doc/descriptors.md":
(https://github.com/bitcoin/bitcoin/pull/33986#discussion_r2615124857)
Looks like this one only has a markdown version, link gives a 404.
(https://github.com/bitcoin/bitcoin/pull/33986#discussion_r2615124857)
Looks like this one only has a markdown version, link gives a 404.
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615178895)
> The problem is that in the rare case where the connection does not conclude with `PING`
I don't think this is a rare case at all. It happens frequently where a connection is aborted early e.g. during handshake. In these cases we won't have a tx either, so the proposed solution would not open a new connection either and make private broadcast be less reliable.
I think we should leave this as is. The race condition that is described here is actually the rare occurrence. I did not experienc
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615178895)
> The problem is that in the rare case where the connection does not conclude with `PING`
I don't think this is a rare case at all. It happens frequently where a connection is aborted early e.g. during handshake. In these cases we won't have a tx either, so the proposed solution would not open a new connection either and make private broadcast be less reliable.
I think we should leave this as is. The race condition that is described here is actually the rare occurrence. I did not experienc
...