💬 Sjors commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3479895077)
@plebhash's account has been restored and his comments and issues came back.
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3479895077)
@plebhash's account has been restored and his comments and issues came back.
💬 maflcko commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3479912512)
> If this is only appearing for this target, in new code, why not address it here? Would be good to at least document what it is.
I think it is just one of the many gcc false-positive warnings. Not sure if it makes sense to document or investigate them individually, given that they are hard to reproduce in light of changing the compiler version or the compile options even slightly.
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3479912512)
> If this is only appearing for this target, in new code, why not address it here? Would be good to at least document what it is.
I think it is just one of the many gcc false-positive warnings. Not sure if it makes sense to document or investigate them individually, given that they are hard to reproduce in light of changing the compiler version or the compile options even slightly.
💬 hodlinator commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486058259)
Given the current stats on initializer list formatting, I don't think there's enough reason to open a PR to turn the tide by changing src/.clang-format.
```shell
# BeforeColon (current default in src/.clang-format, style partially objected to in this thread):
$ grep -zoP "\n( {4})+( {2})m_\w+[({]" -r src/ |wc -l
135
# AfterColon (what I expected to be most common):
$ grep -zoP "\n( {4})+m_\w+[({]" -r src/ |wc -l
117
# BeforeComma (BetaMax of initializer lists, leads to smallest di
...
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486058259)
Given the current stats on initializer list formatting, I don't think there's enough reason to open a PR to turn the tide by changing src/.clang-format.
```shell
# BeforeColon (current default in src/.clang-format, style partially objected to in this thread):
$ grep -zoP "\n( {4})+( {2})m_\w+[({]" -r src/ |wc -l
135
# AfterColon (what I expected to be most common):
$ grep -zoP "\n( {4})+m_\w+[({]" -r src/ |wc -l
117
# BeforeComma (BetaMax of initializer lists, leads to smallest di
...
💬 optout21 commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486059320)
The 'Flush for Prune' has changed here to False, and now all lines have False value here. Can the False line be preserved, or is it OK in this sample output?
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486059320)
The 'Flush for Prune' has changed here to False, and now all lines have False value here. Can the False line be preserved, or is it OK in this sample output?
👍 rkrux approved a pull request: "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response"
(https://github.com/bitcoin/bitcoin/pull/32517#pullrequestreview-3410388983)
lgtm ACK 060bb55508245776bb6a39c8b7849769ee588d69
Only minor nits if retouched.
(https://github.com/bitcoin/bitcoin/pull/32517#pullrequestreview-3410388983)
lgtm ACK 060bb55508245776bb6a39c8b7849769ee588d69
Only minor nits if retouched.
💬 rkrux commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2486017553)
In d633db54166497685b80a12c51db6772982e01fe "rpc: add "ischange: true" in wallet gettransaction decoded tx output"
Nit: #IWYU
```diff
diff --git a/src/core_io.h b/src/core_io.h
index 1874c93a05..57ae6a2af7 100644
--- a/src/core_io.h
+++ b/src/core_io.h
@@ -11,6 +11,7 @@
#include <string>
#include <vector>
#include <optional>
+#include <functional>
class CBlock;
class CBlockHeader;
```
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2486017553)
In d633db54166497685b80a12c51db6772982e01fe "rpc: add "ischange: true" in wallet gettransaction decoded tx output"
Nit: #IWYU
```diff
diff --git a/src/core_io.h b/src/core_io.h
index 1874c93a05..57ae6a2af7 100644
--- a/src/core_io.h
+++ b/src/core_io.h
@@ -11,6 +11,7 @@
#include <string>
#include <vector>
#include <optional>
+#include <functional>
class CBlock;
class CBlockHeader;
```
💬 rkrux commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2486062667)
In 060bb55508245776bb6a39c8b7849769ee588d69 "rpc: add decoded tx details to gettransaction with extra wallet fields"
Nit: Can do away with the word script
```diff
@@ -373,7 +373,7 @@ std::vector<RPCResult> DecodeTxDoc(const std::string& txid_field_doc, bool walle
{RPCResult::Type::OBJ, "scriptPubKey", "", ScriptPubKeyDoc()},
},
wallet ?
- std::vector<RPCResult>{{RPCResult::Type::BOOL, "ischange", /*optional=*/
...
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2486062667)
In 060bb55508245776bb6a39c8b7849769ee588d69 "rpc: add decoded tx details to gettransaction with extra wallet fields"
Nit: Can do away with the word script
```diff
@@ -373,7 +373,7 @@ std::vector<RPCResult> DecodeTxDoc(const std::string& txid_field_doc, bool walle
{RPCResult::Type::OBJ, "scriptPubKey", "", ScriptPubKeyDoc()},
},
wallet ?
- std::vector<RPCResult>{{RPCResult::Type::BOOL, "ischange", /*optional=*/
...
💬 rkrux commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2486019184)
In d633db54166497685b80a12c51db6772982e01fe "rpc: add "ischange: true" in wallet gettransaction decoded tx output"
Nit: s/is_change_func/is_output_change_func
```diff
@@ -47,6 +48,6 @@ std::string FormatScript(const CScript& script);
std::string EncodeHexTx(const CTransaction& tx);
std::string SighashToStr(unsigned char sighash_type);
void ScriptToUniv(const CScript& script, UniValue& out, bool include_hex = true, bool include_address = false, const SigningProvider* provider = nullp
...
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2486019184)
In d633db54166497685b80a12c51db6772982e01fe "rpc: add "ischange: true" in wallet gettransaction decoded tx output"
Nit: s/is_change_func/is_output_change_func
```diff
@@ -47,6 +48,6 @@ std::string FormatScript(const CScript& script);
std::string EncodeHexTx(const CTransaction& tx);
std::string SighashToStr(unsigned char sighash_type);
void ScriptToUniv(const CScript& script, UniValue& out, bool include_hex = true, bool include_address = false, const SigningProvider* provider = nullp
...
💬 optout21 commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#issuecomment-3479931118)
utACK a59aacefd1a0cdb448c8f847665a5809f9897605
(https://github.com/bitcoin/bitcoin/pull/33680#issuecomment-3479931118)
utACK a59aacefd1a0cdb448c8f847665a5809f9897605
💬 l0rinc commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486073173)
This is the actual output of running the script, looks like it wasn't updated in a while
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486073173)
This is the actual output of running the script, looks like it wasn't updated in a while
💬 fanquake commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3479940130)
> I think it is just
If it had been documented (in the commit or PR description), we wouldn't need to guess. Doing that seems like the minimum that should be done, when adding new suppressions; rather than supplying no information, and claiming someone should look in a followup.
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3479940130)
> I think it is just
If it had been documented (in the commit or PR description), we wouldn't need to guess. Doing that seems like the minimum that should be done, when adding new suppressions; rather than supplying no information, and claiming someone should look in a followup.
💬 hodlinator commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486100613)
Agree latest push drops part of the change from bottom of https://github.com/bitcoin/bitcoin/pull/25968#discussion_r963634928. Would prefer this squashed into the last commit:
```diff
--- a/src/test/headers_sync_chainwork_tests.cpp
+++ b/src/test/headers_sync_chainwork_tests.cpp
@@ -52,7 +52,7 @@ constexpr size_t COMMITMENT_PERIOD{600}; // Somewhat close to mainnet.
struct HeadersGeneratorSetup : public RegTestingSetup {
const CBlock& genesis{Params().GenesisBlock()};
- const
...
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486100613)
Agree latest push drops part of the change from bottom of https://github.com/bitcoin/bitcoin/pull/25968#discussion_r963634928. Would prefer this squashed into the last commit:
```diff
--- a/src/test/headers_sync_chainwork_tests.cpp
+++ b/src/test/headers_sync_chainwork_tests.cpp
@@ -52,7 +52,7 @@ constexpr size_t COMMITMENT_PERIOD{600}; // Somewhat close to mainnet.
struct HeadersGeneratorSetup : public RegTestingSetup {
const CBlock& genesis{Params().GenesisBlock()};
- const
...
💬 optout21 commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486109704)
I was wondering how tracing is affected by this change. The trace includes the mode, and it should display it fine, so the trace should be OK.
It would be nice to check how the trace looks for a FORCE_SYNC case as well; I guess that can be triggered by an RPC call to the running node.
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486109704)
I was wondering how tracing is affected by this change. The trace includes the mode, and it should display it fine, so the trace should be OK.
It would be nice to check how the trace looks for a FORCE_SYNC case as well; I guess that can be triggered by an RPC call to the running node.
💬 l0rinc commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486117743)
I've never used tracing before, my understanding is that @0xB10C uses these for monitoring mostly.
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486117743)
I've never used tracing before, my understanding is that @0xB10C uses these for monitoring mostly.
💬 TheCharlatan commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486129322)
I'm observing a memory leak in this fuzz test similar to the one we had for the thread pool. Over there, we disabled logging and instantiate only a single pool instance: https://github.com/bitcoin/bitcoin/pull/33689/files#diff-68602d972fe2b027e3987eff0042c27f3f00fc161b7fe871bdb147571f348298R49-R58 . Maybe similar things can be done here?
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486129322)
I'm observing a memory leak in this fuzz test similar to the one we had for the thread pool. Over there, we disabled logging and instantiate only a single pool instance: https://github.com/bitcoin/bitcoin/pull/33689/files#diff-68602d972fe2b027e3987eff0042c27f3f00fc161b7fe871bdb147571f348298R49-R58 . Maybe similar things can be done here?
📝 l0rinc opened a pull request: "refactor: remove dead branches in `SingletonClusterImpl`"
(https://github.com/bitcoin/bitcoin/pull/33768)
`SplitAll()` always calls `ApplyRemovals()` first, for a singleton, it empties the cluster, therefore any `SingletonClusterImpl` passed to `Split()` must be empty.
`TxGraphImpl::ApplyDependencies()` first merges each dependency group and asserts the group has at least one dependency. Since `parent` != `child`, `TxGraphImpl::Merge()` upgrades the merge target to `GenericClusterImpl`, therefore the `ApplyDependencies()` is never dispatched to `SingletonClusterImpl`.
Found during review: http
...
(https://github.com/bitcoin/bitcoin/pull/33768)
`SplitAll()` always calls `ApplyRemovals()` first, for a singleton, it empties the cluster, therefore any `SingletonClusterImpl` passed to `Split()` must be empty.
`TxGraphImpl::ApplyDependencies()` first merges each dependency group and asserts the group has at least one dependency. Since `parent` != `child`, `TxGraphImpl::Merge()` upgrades the merge target to `GenericClusterImpl`, therefore the `ApplyDependencies()` is never dispatched to `SingletonClusterImpl`.
Found during review: http
...
💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2486133442)
pushed a follow-up to https://github.com/bitcoin/bitcoin/pull/33768
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2486133442)
pushed a follow-up to https://github.com/bitcoin/bitcoin/pull/33768
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486137554)
Thanks for reporting, that explains why I was seeing
```bash
bench_bitcoin(10874,0x207c84800) malloc: Failed to allocate segment from range group - out of space
```
and
```bash
bitcoind(70369,0x16f5ff000) malloc: Failed to allocate segment from range group - out of space
```
recently (cc: @maflcko)
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486137554)
Thanks for reporting, that explains why I was seeing
```bash
bench_bitcoin(10874,0x207c84800) malloc: Failed to allocate segment from range group - out of space
```
and
```bash
bitcoind(70369,0x16f5ff000) malloc: Failed to allocate segment from range group - out of space
```
recently (cc: @maflcko)
💬 maflcko commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#issuecomment-3480077208)
Sorry for missing the review request earlier.
review ACK 060bb55508245776bb6a39c8b7849769ee588d69 🌛
<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+krxU1A3Yux4bpwZNLvVBK
...
(https://github.com/bitcoin/bitcoin/pull/32517#issuecomment-3480077208)
Sorry for missing the review request earlier.
review ACK 060bb55508245776bb6a39c8b7849769ee588d69 🌛
<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+krxU1A3Yux4bpwZNLvVBK
...
💬 optout21 commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486176254)
> This is the actual output of running the script, looks like it wasn't updated in a while
I think that the scripts only enables the trace and listens to it, but it does not trigger a particular use case.
If trace outputs are displayed with these changes, that should be sufficient check (i.e., tracing a FORCE_SYNC is not crucial).
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486176254)
> This is the actual output of running the script, looks like it wasn't updated in a while
I think that the scripts only enables the trace and listens to it, but it does not trigger a particular use case.
If trace outputs are displayed with these changes, that should be sufficient check (i.e., tracing a FORCE_SYNC is not crucial).