🤔 maflcko reviewed a pull request: "fuzz: improve utxo_snapshot target"
(https://github.com/bitcoin/bitcoin/pull/30329#pullrequestreview-2156716926)
ACK b9ba1a73094f4ad593b527e23d2681f41c22539 🎯
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK b9ba1a73094f4ad593b527e23d2
...
(https://github.com/bitcoin/bitcoin/pull/30329#pullrequestreview-2156716926)
ACK b9ba1a73094f4ad593b527e23d2681f41c22539 🎯
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK b9ba1a73094f4ad593b527e23d2
...
👍 theStack approved a pull request: "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB"
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2156741773)
Code-review ACK 8ce3739edbcf6437bf2695087e0ebe8c633df19b
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2156741773)
Code-review ACK 8ce3739edbcf6437bf2695087e0ebe8c633df19b
💬 theStack commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1664366237)
nit: was about to suggest to deduplicate shared code between this method and `::GetLegacyScriptPubKeyMan` above, but since the latter is removed soon anyway (in PR #28710, commit https://github.com/bitcoin/bitcoin/pull/28710/commits/5652a9c59f9ec793886eff00126c382003e4ce02#diff-1f2db0e4d5c12d109c7f0962333c245b49b696cb39ff432da048e9d6c08944d8), it's probably not worth it.
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1664366237)
nit: was about to suggest to deduplicate shared code between this method and `::GetLegacyScriptPubKeyMan` above, but since the latter is removed soon anyway (in PR #28710, commit https://github.com/bitcoin/bitcoin/pull/28710/commits/5652a9c59f9ec793886eff00126c382003e4ce02#diff-1f2db0e4d5c12d109c7f0962333c245b49b696cb39ff432da048e9d6c08944d8), it's probably not worth it.
💬 furszy commented on pull request "[WIP] p2p: send not_found msgs for unknown, pruned or unwilling to share blocks":
(https://github.com/bitcoin/bitcoin/pull/30385#issuecomment-2206523377)
> > The peer who advertised the header might not be the one who ends up providing the final block.
>
> I don't think we ever ask for a block from a peer without that specific peer having announced the block or a descendant to us?
Yeah. I actually spent some time checking that specifically because it would allow an easy way to partition the network by sending only the new header tip, disconnecting and expect the peer requests it to someone else who does not have it etc..
What I meant wit
...
(https://github.com/bitcoin/bitcoin/pull/30385#issuecomment-2206523377)
> > The peer who advertised the header might not be the one who ends up providing the final block.
>
> I don't think we ever ask for a block from a peer without that specific peer having announced the block or a descendant to us?
Yeah. I actually spent some time checking that specifically because it would allow an easy way to partition the network by sending only the new header tip, disconnecting and expect the peer requests it to someone else who does not have it etc..
What I meant wit
...
💬 m3dwards commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#issuecomment-2206597223)
Windows CI fails because the error that I'm looking for `EAI_ADDRFAMILY` doesn't exist in Windows CRT.
Trying to think what to do about it.
(https://github.com/bitcoin/bitcoin/pull/30245#issuecomment-2206597223)
Windows CI fails because the error that I'm looking for `EAI_ADDRFAMILY` doesn't exist in Windows CRT.
Trying to think what to do about it.
💬 ryanofsky commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2206612992)
> Not really convinced explaining things is getting anywhere? Sometimes you have to try fully implementing your idea before you realise it doesn't work...
I think #30342 is a full implementation of making it possible the specify log instances kernel code should use. If you are looking for an implementation of a change to remove the hardcoded global logger instance from the kernel (replacing it as described with a global pointer to a default fallback log instance), I'll provide that, and hopef
...
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2206612992)
> Not really convinced explaining things is getting anywhere? Sometimes you have to try fully implementing your idea before you realise it doesn't work...
I think #30342 is a full implementation of making it possible the specify log instances kernel code should use. If you are looking for an implementation of a change to remove the hardcoded global logger instance from the kernel (replacing it as described with a global pointer to a default fallback log instance), I'll provide that, and hopef
...
🚀 glozow merged a pull request: "26.2 final changes"
(https://github.com/bitcoin/bitcoin/pull/30376)
(https://github.com/bitcoin/bitcoin/pull/30376)
💬 jadijadi commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#issuecomment-2206679066)
@luke-jr May I kindly ask for a review on this PR?
(https://github.com/bitcoin-core/gui/pull/626#issuecomment-2206679066)
@luke-jr May I kindly ask for a review on this PR?
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664435059)
I think the equivalent assert would be
```
BOOST_CHECK(n2.second.IsFresh() && !n2.second.IsDirty());
```
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664435059)
I think the equivalent assert would be
```
BOOST_CHECK(n2.second.IsFresh() && !n2.second.IsDirty());
```
📝 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.