💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3479848795)
Code review reACK 8a4ab45c13dd7ca4474bc4621110da82e0a634f8
I have re-rebased [my previous ack](https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3208833064), solved the conflicts and did a sort reset compared to this, the only differences were whitespace changes, comment fixes, [static cast update](https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2269932154).
Not sure if https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2485972867 was on purpose or not, but either i
...
(https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3479848795)
Code review reACK 8a4ab45c13dd7ca4474bc4621110da82e0a634f8
I have re-rebased [my previous ack](https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3208833064), solved the conflicts and did a sort reset compared to this, the only differences were whitespace changes, comment fixes, [static cast update](https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2269932154).
Not sure if https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2485972867 was on purpose or not, but either i
...
🤔 maflcko reviewed a pull request: "ci: Add Windows + UCRT jobs for cross-compiling and native testing"
(https://github.com/bitcoin/bitcoin/pull/33764#pullrequestreview-3410406728)
should the depends build instructions be updated to mention this?
(https://github.com/bitcoin/bitcoin/pull/33764#pullrequestreview-3410406728)
should the depends build instructions be updated to mention this?
💬 maflcko commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2486031033)
nit: Could consider naming this `ci_win64` (and the file), as this will be the default eventually, going forward.
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2486031033)
nit: Could consider naming this `ci_win64` (and the file), as this will be the default eventually, going forward.
💬 fanquake commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3479891594)
> A new -Warray-bounds compiler diagnostic should be evaluated and addressed.
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.
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3479891594)
> A new -Warray-bounds compiler diagnostic should be evaluated and addressed.
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.
💬 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
...