💬 stickies-v commented on pull request "[27.x] More backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/29780#discussion_r1564907583)
"This new format" right after talking about the legacy format is consusing imo.
Suggested rewrite 1)first explains the new format, 2) then describes how to fall back, 3) adds the PR number:
```
- The `mempool.dat` file created by -persistmempool or the savemempool RPC will
be written in a new format. This new format includes the XOR'ing of transaction
contents to mitigate issues where external programs (such as anti-virus) attempt
to interpret and potentially modify the file.
...
(https://github.com/bitcoin/bitcoin/pull/29780#discussion_r1564907583)
"This new format" right after talking about the legacy format is consusing imo.
Suggested rewrite 1)first explains the new format, 2) then describes how to fall back, 3) adds the PR number:
```
- The `mempool.dat` file created by -persistmempool or the savemempool RPC will
be written in a new format. This new format includes the XOR'ing of transaction
contents to mitigate issues where external programs (such as anti-virus) attempt
to interpret and potentially modify the file.
...
💬 laanwj commented on pull request "build: special instruction check script":
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-2054193483)
> cc @laanwj you might have some thoughts here?
i think this script makes sense, it's good to check that objects with special instructions don't contain a startup section (that will always run). Agree it should be part of the build process (guix at least), and ideally the CI, to be useful.
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-2054193483)
> cc @laanwj you might have some thoughts here?
i think this script makes sense, it's good to check that objects with special instructions don't contain a startup section (that will always run). Agree it should be part of the build process (guix at least), and ideally the CI, to be useful.
💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1564950403)
My previous comments were a bit misleading because only [`%SystemRoot%`](https://learn.microsoft.com/en-us/windows/deployment/usmt/usmt-recognized-environment-variables) is required to be passed through into a subprocess.
From the Python [docs](https://docs.python.org/3/library/subprocess.html):
> If specified, `env` must provide any variables required for the program to execute. On Windows, in order to run a [side-by-side assembly](https://en.wikipedia.org/wiki/Side-by-Side_Assembly) the sp
...
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1564950403)
My previous comments were a bit misleading because only [`%SystemRoot%`](https://learn.microsoft.com/en-us/windows/deployment/usmt/usmt-recognized-environment-variables) is required to be passed through into a subprocess.
From the Python [docs](https://docs.python.org/3/library/subprocess.html):
> If specified, `env` must provide any variables required for the program to execute. On Windows, in order to run a [side-by-side assembly](https://en.wikipedia.org/wiki/Side-by-Side_Assembly) the sp
...
💬 hebasto commented on pull request "[WIP] build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2054214065)
> Concept ACK. Looking at the [leveldb godbolt link](https://godbolt.org/z/45S0ID), this is nicely optimized everywhere except MSVC.
However, the changes in MSVC generated assembly code look quite significant.
> I'm ok with a possible regression there for the sake of the cleanup.
I disagree. Before stacking another performance deterioration change on top of the pile of the currently unresolved performance issues in the MSVC builds, it would be nice to compare benchmarks in the first pla
...
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2054214065)
> Concept ACK. Looking at the [leveldb godbolt link](https://godbolt.org/z/45S0ID), this is nicely optimized everywhere except MSVC.
However, the changes in MSVC generated assembly code look quite significant.
> I'm ok with a possible regression there for the sake of the cleanup.
I disagree. Before stacking another performance deterioration change on top of the pile of the currently unresolved performance issues in the MSVC builds, it would be nice to compare benchmarks in the first pla
...
🤔 tdb3 reviewed a pull request: "rpc: return warnings as an array instead of just a single one"
(https://github.com/bitcoin/bitcoin/pull/29845#pullrequestreview-1999801519)
ACK for 4b821915bf92082043d6cf60efb1a5faea0151db
Nice addition and great opportunistic cleanup of `GetWarnings()`.
Built and ran all unit and functional tests (all passed).
On signet, tested by running `getblockchaininfo` with
- no warnings, by supressing the pre-release warning with `if (false && !CLIENT_VERSION_IS_RELEASE)` and rebuilding,
- one warning (the pre-release warning)
- two warnings, the pre-release warning and date/time warning by manually setting the system clock back
...
(https://github.com/bitcoin/bitcoin/pull/29845#pullrequestreview-1999801519)
ACK for 4b821915bf92082043d6cf60efb1a5faea0151db
Nice addition and great opportunistic cleanup of `GetWarnings()`.
Built and ran all unit and functional tests (all passed).
On signet, tested by running `getblockchaininfo` with
- no warnings, by supressing the pre-release warning with `if (false && !CLIENT_VERSION_IS_RELEASE)` and rebuilding,
- one warning (the pre-release warning)
- two warnings, the pre-release warning and date/time warning by manually setting the system clock back
...
💬 tdb3 commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1565033003)
nit: Looks like the old function comment header used Doxygen syntax. Would recommend keeping this style, but feel free to disregard this nit since the return type seems straightforward, and the function is small/digestible.
```diff
diff --git a/src/warnings.h b/src/warnings.h
index 0b8eb2c9a1..4ae05d4862 100644
--- a/src/warnings.h
+++ b/src/warnings.h
@@ -12,7 +12,10 @@ struct bilingual_str;
void SetMiscWarning(const bilingual_str& warning);
void SetfLargeWorkInvalidChainFound(
...
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1565033003)
nit: Looks like the old function comment header used Doxygen syntax. Would recommend keeping this style, but feel free to disregard this nit since the return type seems straightforward, and the function is small/digestible.
```diff
diff --git a/src/warnings.h b/src/warnings.h
index 0b8eb2c9a1..4ae05d4862 100644
--- a/src/warnings.h
+++ b/src/warnings.h
@@ -12,7 +12,10 @@ struct bilingual_str;
void SetMiscWarning(const bilingual_str& warning);
void SetfLargeWorkInvalidChainFound(
...
💬 laanwj commented on issue "contrib: add symbol-check test for non-existence of `vmova` instructions in Windows build":
(https://github.com/bitcoin/bitcoin/issues/28413#issuecomment-2054821617)
For reference, with capstone this check is as simple as:
```python3
# Intel® 64 and IA-32 Architectures Software Developer’s Manual:
# chapter 14.9, table 14-22. Instructions Requiring Explicitly Aligned Memory
# chapter 15.7, Table 15-6. SIMD Instructions Requiring Explicitly Aligned Memory
#
# This amounts to the following instructions:
#
# instruction chapter 4.3 section
# --------------------------- ---------------------------------
# (V)MOVDQA xmm, mBBB
...
(https://github.com/bitcoin/bitcoin/issues/28413#issuecomment-2054821617)
For reference, with capstone this check is as simple as:
```python3
# Intel® 64 and IA-32 Architectures Software Developer’s Manual:
# chapter 14.9, table 14-22. Instructions Requiring Explicitly Aligned Memory
# chapter 15.7, Table 15-6. SIMD Instructions Requiring Explicitly Aligned Memory
#
# This amounts to the following instructions:
#
# instruction chapter 4.3 section
# --------------------------- ---------------------------------
# (V)MOVDQA xmm, mBBB
...
💬 maflcko commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2055816697)
Did someone with Windows try to enable the benchmarks in guix and compile them, and run them?
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2055816697)
Did someone with Windows try to enable the benchmarks in guix and compile them, and run them?
✅ dergoegge closed an issue: "Increase fuzz coverage in the wallet"
(https://github.com/bitcoin/bitcoin/issues/27272)
(https://github.com/bitcoin/bitcoin/issues/27272)
💬 dergoegge commented on issue "Increase fuzz coverage in the wallet":
(https://github.com/bitcoin/bitcoin/issues/27272#issuecomment-2055826538)
@brunoerg is planning on opening a new issue which will keep better track of the progress on wallet fuzzing.
(https://github.com/bitcoin/bitcoin/issues/27272#issuecomment-2055826538)
@brunoerg is planning on opening a new issue which will keep better track of the progress on wallet fuzzing.
👍 rkrux approved a pull request: "test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply"
(https://github.com/bitcoin/bitcoin/pull/29617#pullrequestreview-2000245020)
tACK [e807838](https://github.com/bitcoin/bitcoin/pull/29617/commits/e807838d26d81de916e9788676fe75d4cb01f6a8)
I was able to successfully build and run all the functional tests.
Provided a refactoring suggestion in favour of clear separation of variables.
(https://github.com/bitcoin/bitcoin/pull/29617#pullrequestreview-2000245020)
tACK [e807838](https://github.com/bitcoin/bitcoin/pull/29617/commits/e807838d26d81de916e9788676fe75d4cb01f6a8)
I was able to successfully build and run all the functional tests.
Provided a refactoring suggestion in favour of clear separation of variables.
💬 rkrux commented on pull request "test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply":
(https://github.com/bitcoin/bitcoin/pull/29617#discussion_r1565319719)
Suggest the below refactoring to avoid passing wrong_hash and error_message in the same variable while relying on is_custom_message to differentiate between the two.
```
diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index a70b511c91..d07f2db530 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -100,25 +100,24 @@ class AssumeutxoTest(BitcoinTestFramework):
self.log.info(" - snapshot fil
...
(https://github.com/bitcoin/bitcoin/pull/29617#discussion_r1565319719)
Suggest the below refactoring to avoid passing wrong_hash and error_message in the same variable while relying on is_custom_message to differentiate between the two.
```
diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index a70b511c91..d07f2db530 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -100,25 +100,24 @@ class AssumeutxoTest(BitcoinTestFramework):
self.log.info(" - snapshot fil
...
📝 dergoegge opened a pull request: "rpc, bugfix: Enforce maximum value for setmocktime"
(https://github.com/bitcoin/bitcoin/pull/29869)
The maximum value for our mocktime must be representable in nanoseconds, otherwise we end up with negative values returned from `NodeClock::now()`.
Found through fuzzing:
```
$ echo "c2V0bW9ja3RpbWVcZTptYf9w/3NldG3///////////////9p////ZP///ymL//////89////Nv9L////////LXkBAABpAA==" | base64 --decode > rpc-8cab9148ab4418ebd1923c213e9d3fe9c9b49b39.crash
$ FUZZ=rpc ./src/test/fuzz/fuzz rpc-8cab9148ab4418ebd1923c213e9d3fe9c9b49b39.crash
fuzz_libfuzzer: util/time.cpp:28: static NodeClock::time
...
(https://github.com/bitcoin/bitcoin/pull/29869)
The maximum value for our mocktime must be representable in nanoseconds, otherwise we end up with negative values returned from `NodeClock::now()`.
Found through fuzzing:
```
$ echo "c2V0bW9ja3RpbWVcZTptYf9w/3NldG3///////////////9p////ZP///ymL//////89////Nv9L////////LXkBAABpAA==" | base64 --decode > rpc-8cab9148ab4418ebd1923c213e9d3fe9c9b49b39.crash
$ FUZZ=rpc ./src/test/fuzz/fuzz rpc-8cab9148ab4418ebd1923c213e9d3fe9c9b49b39.crash
fuzz_libfuzzer: util/time.cpp:28: static NodeClock::time
...
📝 maflcko opened a pull request: "rpc: Reword SighashFromStr error message"
(https://github.com/bitcoin/bitcoin/pull/29870)
Put quotes around the parameter. In theory, `std::quoted` should be used, but that seems overkill.
This should avoid error messages such as `A valid sighash parameter is not a valid sighash parameter. (code -8)`.
Also, it should fix fuzz false positives when searching for internal bugs in the `rpc` fuzz target. For example, `ZGVzY3JpcHRvcnByb2Nlc3Nwc2J0XP9ce1tdXOVJbnRlcm5hbCBidWcgZGV0ZWN0ZWQAXQ0AHfcAXQ1p7TJv`.
(https://github.com/bitcoin/bitcoin/pull/29870)
Put quotes around the parameter. In theory, `std::quoted` should be used, but that seems overkill.
This should avoid error messages such as `A valid sighash parameter is not a valid sighash parameter. (code -8)`.
Also, it should fix fuzz false positives when searching for internal bugs in the `rpc` fuzz target. For example, `ZGVzY3JpcHRvcnByb2Nlc3Nwc2J0XP9ce1tdXOVJbnRlcm5hbCBidWcgZGV0ZWN0ZWQAXQ0AHfcAXQ1p7TJv`.
👍 maflcko approved a pull request: "rpc, bugfix: Enforce maximum value for setmocktime"
(https://github.com/bitcoin/bitcoin/pull/29869#pullrequestreview-2000309755)
Could combine the two error messages into one, as both are related to out-of-range?
(https://github.com/bitcoin/bitcoin/pull/29869#pullrequestreview-2000309755)
Could combine the two error messages into one, as both are related to out-of-range?
💬 maflcko commented on pull request "rpc, bugfix: Enforce maximum value for setmocktime":
(https://github.com/bitcoin/bitcoin/pull/29869#discussion_r1565359185)
```suggestion
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Mocktime must be in the range [0, %s], not %s.", max_time, time));
```
(https://github.com/bitcoin/bitcoin/pull/29869#discussion_r1565359185)
```suggestion
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Mocktime must be in the range [0, %s], not %s.", max_time, time));
```
💬 dergoegge commented on pull request "rpc, bugfix: Enforce maximum value for setmocktime":
(https://github.com/bitcoin/bitcoin/pull/29869#discussion_r1565362888)
Done.
(https://github.com/bitcoin/bitcoin/pull/29869#discussion_r1565362888)
Done.
👍 maflcko approved a pull request: "rpc, bugfix: Enforce maximum value for setmocktime"
(https://github.com/bitcoin/bitcoin/pull/29869#pullrequestreview-2000318650)
Thanks, lgtm
(https://github.com/bitcoin/bitcoin/pull/29869#pullrequestreview-2000318650)
Thanks, lgtm
💬 maflcko commented on pull request "rpc, bugfix: Enforce maximum value for setmocktime":
(https://github.com/bitcoin/bitcoin/pull/29869#discussion_r1565364205)
nit: could be better to remove this and use `util/time.h` instead?
(https://github.com/bitcoin/bitcoin/pull/29869#discussion_r1565364205)
nit: could be better to remove this and use `util/time.h` instead?
💬 dergoegge commented on pull request "rpc, bugfix: Enforce maximum value for setmocktime":
(https://github.com/bitcoin/bitcoin/pull/29869#discussion_r1565366501)
Done
(https://github.com/bitcoin/bitcoin/pull/29869#discussion_r1565366501)
Done