📝 ajtowns opened a pull request: "Early logging improvements"
(https://github.com/bitcoin/bitcoin/pull/30386)
In order to cope gracefully with `Log*()` calls that are invoked prior to logging being fully configured (indicated by calling `StartLogging()` we buffer early log messages in `m_msgs_before_open`. This has a couple of minor issues:
* if there are many such log messages the buffer can become arbitrarily large; this can be a problem for users of libkernel that might not wish to worry about logging at all, and as a result never invoke `StartLogging()`
* early log messages are formatted befor
...
(https://github.com/bitcoin/bitcoin/pull/30386)
In order to cope gracefully with `Log*()` calls that are invoked prior to logging being fully configured (indicated by calling `StartLogging()` we buffer early log messages in `m_msgs_before_open`. This has a couple of minor issues:
* if there are many such log messages the buffer can become arbitrarily large; this can be a problem for users of libkernel that might not wish to worry about logging at all, and as a result never invoke `StartLogging()`
* early log messages are formatted befor
...
💬 theStack commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2206760235)
Suggestion for adding corresponding `decodescript` test coverage:
<details>
<summary>Patch</summary>
<br>
```diff
diff --git a/test/functional/rpc_decodescript.py b/test/functional/rpc_decodescript.py
index f37e61ab50..a105b3d3b4 100755
--- a/test/functional/rpc_decodescript.py
+++ b/test/functional/rpc_decodescript.py
@@ -187,6 +187,16 @@ class DecodeScriptTest(BitcoinTestFramework):
assert_equal('1 ' + xonly_public_key, rpc_result['asm'])
assert 'segwit' not in
...
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2206760235)
Suggestion for adding corresponding `decodescript` test coverage:
<details>
<summary>Patch</summary>
<br>
```diff
diff --git a/test/functional/rpc_decodescript.py b/test/functional/rpc_decodescript.py
index f37e61ab50..a105b3d3b4 100755
--- a/test/functional/rpc_decodescript.py
+++ b/test/functional/rpc_decodescript.py
@@ -187,6 +187,16 @@ class DecodeScriptTest(BitcoinTestFramework):
assert_equal('1 ' + xonly_public_key, rpc_result['asm'])
assert 'segwit' not in
...
💬 ajtowns commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2206789244)
> Re [#30338 (comment)](https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197683031)
>
> > Have a look at [ajtowns#9](https://github.com/ajtowns/bitcoin/pull/9) and see what you think;
>
> This looks good to me, thank you for implementing.
Okay, see #30386 for the more serious take. I have another patch that I could add on top that would also buffer early debug/trace logs, but that seemed a bit unnecessary (LMK if it would be useful).
> Regarding disabling the buffering, I
...
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2206789244)
> Re [#30338 (comment)](https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197683031)
>
> > Have a look at [ajtowns#9](https://github.com/ajtowns/bitcoin/pull/9) and see what you think;
>
> This looks good to me, thank you for implementing.
Okay, see #30386 for the more serious take. I have another patch that I could add on top that would also buffer early debug/trace logs, but that seemed a bit unnecessary (LMK if it would be useful).
> Regarding disabling the buffering, I
...
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664508903)
I guess `head` could be though of as a sentinel, which always points to the head of the linked list? I suppose I misnamed that then.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664508903)
I guess `head` could be though of as a sentinel, which always points to the head of the linked list? I suppose I misnamed that then.
💬 ajtowns commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#issuecomment-2206858748)
Easiest way to test is to add some log lines into init.cpp `InitLogging()`, eg:
```c++
for (int i = 0; i < 10000; ++i) {
LogInfo("log spam %d...\n", i);
}
LogWarning("awoogah\n");
LogError("internal bug not detected\nhello, world!\nwas that fun?\n");
```
Running `bitcoind -regtest -logtimemicros` should give a decent indication of the differences.
If you want to modify bitcoin-chainstate to see what the logs would look like, remove the `DisableLogging` call
...
(https://github.com/bitcoin/bitcoin/pull/30386#issuecomment-2206858748)
Easiest way to test is to add some log lines into init.cpp `InitLogging()`, eg:
```c++
for (int i = 0; i < 10000; ++i) {
LogInfo("log spam %d...\n", i);
}
LogWarning("awoogah\n");
LogError("internal bug not detected\nhello, world!\nwas that fun?\n");
```
Running `bitcoind -regtest -logtimemicros` should give a decent indication of the differences.
If you want to modify bitcoin-chainstate to see what the logs would look like, remove the `DisableLogging` call
...
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664539266)
the exact equivalent, yes, but you might not want to test the dirtiness part here - up to you, I don't mind either way.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664539266)
the exact equivalent, yes, but you might not want to test the dirtiness part here - up to you, I don't mind either way.
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664542828)
hmmm, why did this become `=` instead of `|=`?
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664542828)
hmmm, why did this become `=` instead of `|=`?
✅ ajtowns closed a pull request: "logging: Simplify edge cases in logging configuration"
(https://github.com/bitcoin/bitcoin/pull/30384)
(https://github.com/bitcoin/bitcoin/pull/30384)
💬 ajtowns commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#issuecomment-2206882030)
Consider also updating the help text for `-debug` and `-debugexclude`:
```diff
--- a/src/init/common.cpp
+++ b/src/init/common.cpp
@@ -27,9 +27,9 @@ void AddLoggingArgs(ArgsManager& argsman)
{
argsman.AddArg("-debuglogfile=<file>", strprintf("Specify location of debug log file (default: %s). Relative paths will be prefixed by a net-specific datadir location. Pass -nodebuglogfile to disable writing the log to a file.", DEFAULT_DEBUGLOGFILE), ArgsManager::ALLOW_ANY, OptionsCategory::
...
(https://github.com/bitcoin/bitcoin/pull/29798#issuecomment-2206882030)
Consider also updating the help text for `-debug` and `-debugexclude`:
```diff
--- a/src/init/common.cpp
+++ b/src/init/common.cpp
@@ -27,9 +27,9 @@ void AddLoggingArgs(ArgsManager& argsman)
{
argsman.AddArg("-debuglogfile=<file>", strprintf("Specify location of debug log file (default: %s). Relative paths will be prefixed by a net-specific datadir location. Pass -nodebuglogfile to disable writing the log to a file.", DEFAULT_DEBUGLOGFILE), ArgsManager::ALLOW_ANY, OptionsCategory::
...
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664555939)
Whoops bad rebase, thanks for catching!
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664555939)
Whoops bad rebase, thanks for catching!
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664558253)
But since these are unit tests we are kind of testing the internals, so it would be good to catch if any other flags sneak in. With `IsFresh` it just checks that the FRESH flag is set, but any other flag could be set as well.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664558253)
But since these are unit tests we are kind of testing the internals, so it would be good to catch if any other flags sneak in. With `IsFresh` it just checks that the FRESH flag is set, but any other flag could be set as well.
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664558577)
Added `noexcept` everywhere.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664558577)
Added `noexcept` everywhere.
💬 ajtowns commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#issuecomment-2206898641)
ACK df2422c5f91e5b1b42d6b718cc2527dc413712fc ; looks like it behaves correctly to me
(https://github.com/bitcoin/bitcoin/pull/29798#issuecomment-2206898641)
ACK df2422c5f91e5b1b42d6b718cc2527dc413712fc ; looks like it behaves correctly to me
💬 stickies-v commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1664572723)
Is there a reason this can't be simplified to:
```suggestion
if (nHeight % consensusParams.DifficultyAdjustmentInterval() == 0) {
```
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1664572723)
Is there a reason this can't be simplified to:
```suggestion
if (nHeight % consensusParams.DifficultyAdjustmentInterval() == 0) {
```
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664545575)
what happens if this ends up throwing, it's not a trivial function (e.g. `m_prev` could become nullable in the future and cause havoc)?
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664545575)
what happens if this ends up throwing, it's not a trivial function (e.g. `m_prev` could become nullable in the future and cause havoc)?
🤔 paplorinc reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2157030164)
I ran the benchmarks without prune (on the previous push), with just a lazy `-reindex` or `-reindex-chainstate`, but couldn't measure any difference - do I need more than 200k blocks? Or is prune critical here? Or is lazy reindexing not enough?
```bash
sudo ./src/bitcoind -stopatheight=200000
sudo hyperfine \
--warmup 1 --runs 5 \
--show-output \
--parameter-list commit master,670084adc53e3d661ea5b4a19743659609a42d5e \
--prepare 'git checkout {commit} && git reset --hard && make -j10
...
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2157030164)
I ran the benchmarks without prune (on the previous push), with just a lazy `-reindex` or `-reindex-chainstate`, but couldn't measure any difference - do I need more than 200k blocks? Or is prune critical here? Or is lazy reindexing not enough?
```bash
sudo ./src/bitcoind -stopatheight=200000
sudo hyperfine \
--warmup 1 --runs 5 \
--show-output \
--parameter-list commit master,670084adc53e3d661ea5b4a19743659609a42d5e \
--prepare 'git checkout {commit} && git reset --hard && make -j10
...
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664560161)
maybe it could make sense to document these rare conditions with something like `} else if ([[unlikely]] fOk) {`
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664560161)
maybe it could make sense to document these rare conditions with something like `} else if ([[unlikely]] fOk) {`
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664547028)
does the naming inconsistency apply here as well - given that we ignore the first element with instant `.Next()` as a first step?
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664547028)
does the naming inconsistency apply here as well - given that we ignore the first element with instant `.Next()` as a first step?
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664597181)
Yeah I suppose we can rename it `m_flagged_sentinel` or something? Do you have a suggestion that would have made this clearer if that was the name originally?
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664597181)
Yeah I suppose we can rename it `m_flagged_sentinel` or something? Do you have a suggestion that would have made this clearer if that was the name originally?
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2206960214)
> I ran the benchmarks without prune (on the previous push), with just a lazy -reindex or -reindex-chainstate, but couldn't measure any difference - do I need more than 200k blocks? Or is prune critical here? Or is lazy reindexing not enough?
It shouldn't have much effect on non-pruning nodes, so it's good that there's no regression.
You will need pruning enabled to see the performance benefits. The higher the dbcache and lower the prune settings the better. The higher the block height the b
...
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2206960214)
> I ran the benchmarks without prune (on the previous push), with just a lazy -reindex or -reindex-chainstate, but couldn't measure any difference - do I need more than 200k blocks? Or is prune critical here? Or is lazy reindexing not enough?
It shouldn't have much effect on non-pruning nodes, so it's good that there's no regression.
You will need pruning enabled to see the performance benefits. The higher the dbcache and lower the prune settings the better. The higher the block height the b
...