๐ฌ 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
๐ฌ maflcko commented on pull request "index: block filters sync, reduce disk read operations by caching last header":
(https://github.com/bitcoin/bitcoin/pull/28955#issuecomment-2056140745)
Follow-up in https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2054064278
(https://github.com/bitcoin/bitcoin/pull/28955#issuecomment-2056140745)
Follow-up in https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2054064278
๐ฌ ajtowns commented on pull request "add `-limitdummyscriptdatasize` option":
(https://github.com/bitcoin/bitcoin/pull/29520#issuecomment-2056153503)
> Implementing tests are way above want I can do, so it's up for grab. [[ref](https://github.com/bitcoin/bitcoin/pull/29520#discussion_r1543749260)]
I don't think it makes a lot of sense to add features without basic functional tests demonstrating they work as intended. Perhaps someone reading this is up for creating some?
(https://github.com/bitcoin/bitcoin/pull/29520#issuecomment-2056153503)
> Implementing tests are way above want I can do, so it's up for grab. [[ref](https://github.com/bitcoin/bitcoin/pull/29520#discussion_r1543749260)]
I don't think it makes a lot of sense to add features without basic functional tests demonstrating they work as intended. Perhaps someone reading this is up for creating some?
๐ฌ maflcko commented on pull request "rpc, bugfix: Enforce maximum value for setmocktime":
(https://github.com/bitcoin/bitcoin/pull/29869#issuecomment-2056157190)
lgtm ACK 61df72ee4dee3c5d739a3c851c86fdb4e4c640e5
(https://github.com/bitcoin/bitcoin/pull/29869#issuecomment-2056157190)
lgtm ACK 61df72ee4dee3c5d739a3c851c86fdb4e4c640e5
๐ค fjahr reviewed a pull request: "test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply"
(https://github.com/bitcoin/bitcoin/pull/29617#pullrequestreview-2000312610)
Concept ACK
I think it would be helpful for reviewers if you could add information in the commit message how you arrived at the content values `b"\x84\x58"` and `b"\xCA\xD2\x8F\x5A"`.
(https://github.com/bitcoin/bitcoin/pull/29617#pullrequestreview-2000312610)
Concept ACK
I think it would be helpful for reviewers if you could add information in the commit message how you arrived at the content values `b"\x84\x58"` and `b"\xCA\xD2\x8F\x5A"`.
๐ฌ fjahr 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_r1565361998)
Unnecessary blank space removal
(https://github.com/bitcoin/bitcoin/pull/29617#discussion_r1565361998)
Unnecessary blank space removal
๐ฌ fjahr 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_r1565360894)
I think using one field for multiple things makes this more confusing than necessary. Please keep the hash field for the hash and where you have the boolean you can use that field for a custom message. Then you can check if the custom message is set instead of an explicit boolean.
(https://github.com/bitcoin/bitcoin/pull/29617#discussion_r1565360894)
I think using one field for multiple things makes this more confusing than necessary. Please keep the hash field for the hash and where you have the boolean you can use that field for a custom message. Then you can check if the custom message is set instead of an explicit boolean.