💬 A-Manning commented on pull request "rest: Query predecessor headers using negative count param":
(https://github.com/bitcoin/bitcoin/pull/33752#issuecomment-3479817272)
>Though, I wonder what the use case is. If someone knows a header, it seems likely they also know about the previous headers and a feature to support asking for them would have no users?
Knowing a header only means that you know the previous block hash. If you want to request headers from best tip to genesis, There is no existing mechanism other than requesting headers one-at-a-time. This makes it possible to *batch* request headers from tip to genesis, which is much more efficient than reque
...
(https://github.com/bitcoin/bitcoin/pull/33752#issuecomment-3479817272)
>Though, I wonder what the use case is. If someone knows a header, it seems likely they also know about the previous headers and a feature to support asking for them would have no users?
Knowing a header only means that you know the previous block hash. If you want to request headers from best tip to genesis, There is no existing mechanism other than requesting headers one-at-a-time. This makes it possible to *batch* request headers from tip to genesis, which is much more efficient than reque
...
💬 maflcko commented on pull request "ci: Call docker exec from Python script to fix word splitting":
(https://github.com/bitcoin/bitcoin/pull/33732#discussion_r2485987246)
This is just moving the code one-to-one, but possibly `rsync`, along with the rsync command passing, could be required here. (IIRC the error message may have been better worded to reflect the intention: `rsync not found, no need to copy from ...`)
At least on macOS, it seems to pass: https://github.com/bitcoin/bitcoin/actions/runs/18913390006/job/53990076461?pr=33732#step:7:134
```
+ rsync --recursive --perms --stats --human-readable /Users/runner/work/bitcoin/bitcoin/ /Users/runner/work/
...
(https://github.com/bitcoin/bitcoin/pull/33732#discussion_r2485987246)
This is just moving the code one-to-one, but possibly `rsync`, along with the rsync command passing, could be required here. (IIRC the error message may have been better worded to reflect the intention: `rsync not found, no need to copy from ...`)
At least on macOS, it seems to pass: https://github.com/bitcoin/bitcoin/actions/runs/18913390006/job/53990076461?pr=33732#step:7:134
```
+ rsync --recursive --perms --stats --human-readable /Users/runner/work/bitcoin/bitcoin/ /Users/runner/work/
...
💬 maflcko commented on pull request "ci: Call docker exec from Python script to fix word splitting":
(https://github.com/bitcoin/bitcoin/pull/33732#discussion_r2485988430)
(added a commit to require rsync)
(https://github.com/bitcoin/bitcoin/pull/33732#discussion_r2485988430)
(added a commit to require rsync)
💬 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.