💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712435380)
Thanks for your input. I wasn't sure about this either. See https://github.com/bitcoin/bitcoin/pull/30569/#discussion_r1703942199 for previous discussion on this topic. It seems example values have all been 64 characters in the past, and given that it's only used in tests it doesn't seem unreasonably to be a bit more strict to simplify the logic, but I appreciate your point of view too.
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712435380)
Thanks for your input. I wasn't sure about this either. See https://github.com/bitcoin/bitcoin/pull/30569/#discussion_r1703942199 for previous discussion on this topic. It seems example values have all been 64 characters in the past, and given that it's only used in tests it doesn't seem unreasonably to be a bit more strict to simplify the logic, but I appreciate your point of view too.
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712432448)
> 0x which isn't mentioned in the commit message, and also reject numbers with whitespace prefixes and suffixes and trailing non-hex characters.
`x`, whitespace, and trailing non-hex characters are all non-hex characters, so arguably it is mentioned in the commit message, but I agree it wouldn't hurt to be explicit about the 0x prefix and whitespace.
> and it also seems ok to not require numbers to be padded with leading 0's.
I'm a bit confused about this comment being in the `-assumeva
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712432448)
> 0x which isn't mentioned in the commit message, and also reject numbers with whitespace prefixes and suffixes and trailing non-hex characters.
`x`, whitespace, and trailing non-hex characters are all non-hex characters, so arguably it is mentioned in the commit message, but I agree it wouldn't hurt to be explicit about the 0x prefix and whitespace.
> and it also seems ok to not require numbers to be padded with leading 0's.
I'm a bit confused about this comment being in the `-assumeva
...
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712427499)
> The number is not just rejected when it is too long but also if it has whitespace prefixes or suffixes or trailing non-hex characters. These would have been ignored previously.
Is that so? It seems to me that `IsHexNumber()` would have returned false for all of those cases, which is why 802374b4355bd1dec7a88bba6287c55f935699fe is a refactor commit?
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712427499)
> The number is not just rejected when it is too long but also if it has whitespace prefixes or suffixes or trailing non-hex characters. These would have been ignored previously.
Is that so? It seems to me that `IsHexNumber()` would have returned false for all of those cases, which is why 802374b4355bd1dec7a88bba6287c55f935699fe is a refactor commit?
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712440706)
Ah yes you're right, this is to follow existing behaviour of `IsHexNumber()` but it's not documented, I'll fix that, thanks.
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712440706)
Ah yes you're right, this is to follow existing behaviour of `IsHexNumber()` but it's not documented, I'll fix that, thanks.
💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712563888)
> In commit "test: Only accept a fully valid RANDOM_CTX_SEED" ([855784d](https://github.com/bitcoin/bitcoin/commit/855784d3a0026414159acc42fceeb271f8a28133))
>
> I don't understand the motivation for being so strict here.
Just for reference, you approved the exact same commit 2 days prior in https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2222184242 .
Maybe my comment from https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706570743 can be moved into the commit m
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712563888)
> In commit "test: Only accept a fully valid RANDOM_CTX_SEED" ([855784d](https://github.com/bitcoin/bitcoin/commit/855784d3a0026414159acc42fceeb271f8a28133))
>
> I don't understand the motivation for being so strict here.
Just for reference, you approved the exact same commit 2 days prior in https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2222184242 .
Maybe my comment from https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706570743 can be moved into the commit m
...
💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2279822735)
> Because my understanding is that `0x` is used for hex numbers, and a block hash is not a number.
If it's not a number, why are we prefixing it with 0 (by a method called TrySanitizeHex*Number*)?
> parameter of base_blob::FromHex(),
or alternatively chain the two together like `TrimAndParse`, i.e. `base_blob::FromHexLenient()`
> -assumedvalid [...] -minimumchainwork
Would it be clearer if we just warned about the changes in this release and require sanitized input in the next?
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2279822735)
> Because my understanding is that `0x` is used for hex numbers, and a block hash is not a number.
If it's not a number, why are we prefixing it with 0 (by a method called TrySanitizeHex*Number*)?
> parameter of base_blob::FromHex(),
or alternatively chain the two together like `TrimAndParse`, i.e. `base_blob::FromHexLenient()`
> -assumedvalid [...] -minimumchainwork
Would it be clearer if we just warned about the changes in this release and require sanitized input in the next?
💬 Sjors commented on pull request "validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2279837287)
Rebased after the (tiny) format change in #30598, for easier testing. Updated torrent link and snapshot file hash in the description. The chainparams themselves are unchanged.
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2279837287)
Rebased after the (tiny) format change in #30598, for easier testing. Updated torrent link and snapshot file hash in the description. The chainparams themselves are unchanged.
📝 paplorinc opened a pull request: "test: Fuzz the human-readable part of bech32 as well"
(https://github.com/bitcoin/bitcoin/pull/30623)
Split out of https://github.com/bitcoin/bitcoin/pull/30596/files#r1706957219
Instead of the static "bc" human-readable part, it's now randomly generated based on https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki and the extra restrictions in the code:
```
The human-readable part, which is intended to convey the type of data, or anything else that is relevant to the reader. This part MUST contain 1 to 83 US-ASCII characters, with each character having a value in the range [33-12
...
(https://github.com/bitcoin/bitcoin/pull/30623)
Split out of https://github.com/bitcoin/bitcoin/pull/30596/files#r1706957219
Instead of the static "bc" human-readable part, it's now randomly generated based on https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki and the extra restrictions in the code:
```
The human-readable part, which is intended to convey the type of data, or anything else that is relevant to the reader. This part MUST contain 1 to 83 US-ASCII characters, with each character having a value in the range [33-12
...
💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1712594920)
Done in https://github.com/bitcoin/bitcoin/pull/30623#issuecomment-2280136628
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1712594920)
Done in https://github.com/bitcoin/bitcoin/pull/30623#issuecomment-2280136628
💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1712595124)
Should I add this to https://github.com/bitcoin/bitcoin/pull/30623#issuecomment-2280136628 as well?
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1712595124)
Should I add this to https://github.com/bitcoin/bitcoin/pull/30623#issuecomment-2280136628 as well?
💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2280810529)
re-ACK a0abcbd3822bd17a1d73c42ccd5b040a150b0501
Running a fuzzer for a couple days on the previous push (rebased on master to get the speedups from #30197) and with a dictionary specifically targeting the multipath expressions did not uncover anything.
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2280810529)
re-ACK a0abcbd3822bd17a1d73c42ccd5b040a150b0501
Running a fuzzer for a couple days on the previous push (rebased on master to get the speedups from #30197) and with a dictionary specifically targeting the multipath expressions did not uncover anything.
💬 hebasto commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2280829761)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/318.
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2280829761)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/318.
💬 maflcko commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2280879011)
In the meantime I booted up 4 more machines with zram disabled, two of which use three SSDs, as in your setup.
I'd be highly surprised if leveldb corruption has something to do with ipv6, so I won't be testing that.
I'll let them run for a month or two, but if they don't find anything, I am not sure what to do here.
Without *exact* and *full* steps to reproduce every single step, including the exact AWS VM setup, as well as the internal VM setup, there is little that can be done here.
...
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2280879011)
In the meantime I booted up 4 more machines with zram disabled, two of which use three SSDs, as in your setup.
I'd be highly surprised if leveldb corruption has something to do with ipv6, so I won't be testing that.
I'll let them run for a month or two, but if they don't find anything, I am not sure what to do here.
Without *exact* and *full* steps to reproduce every single step, including the exact AWS VM setup, as well as the internal VM setup, there is little that can be done here.
...
💬 maflcko commented on pull request "test: remove `ExtractDestination` false assertion for `ANCHOR` script":
(https://github.com/bitcoin/bitcoin/pull/30616#issuecomment-2280888900)
> (Could wait a day before merging this, to check if OSS-Fuzz also found it, because it should and so far has not)
I guess it may just be non-trivial to find. Let's merge this here, and continue discussion in https://github.com/bitcoin/bitcoin/issues/30615
(https://github.com/bitcoin/bitcoin/pull/30616#issuecomment-2280888900)
> (Could wait a day before merging this, to check if OSS-Fuzz also found it, because it should and so far has not)
I guess it may just be non-trivial to find. Let's merge this here, and continue discussion in https://github.com/bitcoin/bitcoin/issues/30615
💬 maflcko commented on issue "fuzz: `script`: Assertion `!extract_destination_ret' failed.":
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2280919415)
About how easy it is for the fuzz engine to find this bug: This is trivial to find in libFuzzer with a small enough max_len and value profile ( for example, `-use_value_profile=1 -max_len=24`). However, absent that, it seems harder to find on a quick test.
There are no hashes involved here, so in theory symbolic execution should also be able to find this quickly?
Would be fun to use this example for some fuzz benchmarks/evaluation. cc @dergoegge @agroce @murchandamus
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2280919415)
About how easy it is for the fuzz engine to find this bug: This is trivial to find in libFuzzer with a small enough max_len and value profile ( for example, `-use_value_profile=1 -max_len=24`). However, absent that, it seems harder to find on a quick test.
There are no hashes involved here, so in theory symbolic execution should also be able to find this quickly?
Would be fun to use this example for some fuzz benchmarks/evaluation. cc @dergoegge @agroce @murchandamus
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1712605045)
Q: given that FUZZ and BENCH are disabled by default, would it maybe make sense to make them separate projects? I'm not exactly familiar with every single second-order effect of that and don't have strong arguments either way, was just wondering when investigating some other cmake related issue.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1712605045)
Q: given that FUZZ and BENCH are disabled by default, would it maybe make sense to make them separate projects? I'm not exactly familiar with every single second-order effect of that and don't have strong arguments either way, was just wondering when investigating some other cmake related issue.
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1712621112)
Nit, feel free to ignore these if not important:
```suggestion
rpc/spend.cpp
rpc/signmessage.cpp
```
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1712621112)
Nit, feel free to ignore these if not important:
```suggestion
rpc/spend.cpp
rpc/signmessage.cpp
```
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1712605154)
nit: alphabetic ordering?
```patch
diff --git a/src/crypto/CMakeLists.txt b/src/crypto/CMakeLists.txt
--- a/src/crypto/CMakeLists.txt (revision 92f7fc21768b0d898655d68b5d76ff09ff71c682)
+++ b/src/crypto/CMakeLists.txt (date 1723279760256)
@@ -4,19 +4,19 @@
add_library(bitcoin_crypto STATIC EXCLUDE_FROM_ALL
aes.cpp
- chacha20.cpp
chacha20poly1305.cpp
+ chacha20.cpp
hex_base.cpp
hkdf_sha256_32.cpp
hmac_sha256.cpp
hmac_sha512.cpp
- poly1305.cpp
muhash.cpp
...
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1712605154)
nit: alphabetic ordering?
```patch
diff --git a/src/crypto/CMakeLists.txt b/src/crypto/CMakeLists.txt
--- a/src/crypto/CMakeLists.txt (revision 92f7fc21768b0d898655d68b5d76ff09ff71c682)
+++ b/src/crypto/CMakeLists.txt (date 1723279760256)
@@ -4,19 +4,19 @@
add_library(bitcoin_crypto STATIC EXCLUDE_FROM_ALL
aes.cpp
- chacha20.cpp
chacha20poly1305.cpp
+ chacha20.cpp
hex_base.cpp
hkdf_sha256_32.cpp
hmac_sha256.cpp
hmac_sha512.cpp
- poly1305.cpp
muhash.cpp
...
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1712621771)
Nit:
```patch
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
--- a/src/CMakeLists.txt (revision 92f7fc21768b0d898655d68b5d76ff09ff71c682)
+++ b/src/CMakeLists.txt (date 1723286445028)
@@ -106,8 +106,8 @@
addresstype.cpp
base58.cpp
bech32.cpp
- chainparamsbase.cpp
chainparams.cpp
+ chainparamsbase.cpp
coins.cpp
common/args.cpp
common/bloom.cpp
@@ -130,18 +130,18 @@
key.cpp
key_io.cpp
merkleblock.cpp
+ net_permissions.cpp
net_types.cpp
...
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1712621771)
Nit:
```patch
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
--- a/src/CMakeLists.txt (revision 92f7fc21768b0d898655d68b5d76ff09ff71c682)
+++ b/src/CMakeLists.txt (date 1723286445028)
@@ -106,8 +106,8 @@
addresstype.cpp
base58.cpp
bech32.cpp
- chainparamsbase.cpp
chainparams.cpp
+ chainparamsbase.cpp
coins.cpp
common/args.cpp
common/bloom.cpp
@@ -130,18 +130,18 @@
key.cpp
key_io.cpp
merkleblock.cpp
+ net_permissions.cpp
net_types.cpp
...
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1712620747)
> Apparently, not every change invalidates the cache
My understanding is that we're already avoiding the recompilation when the source didn't change, but when `working_linker_werror_flag` changes, that's ignored.
I haven't opened a bug report to CMake since it seems to me we can fix it ourselved, see: https://github.com/hebasto/bitcoin/pull/319
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1712620747)
> Apparently, not every change invalidates the cache
My understanding is that we're already avoiding the recompilation when the source didn't change, but when `working_linker_werror_flag` changes, that's ignored.
I haven't opened a bug report to CMake since it seems to me we can fix it ourselved, see: https://github.com/hebasto/bitcoin/pull/319