💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705375642)
according to the bip the hrp `MUST contain 1 to 83 US-ASCII characters`, maybe we could extend the fuzz testing from the hard-coded "bc"

💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705357479)
nit: According to https://en.bitcoin.it/wiki/BIP_0173 `bc` + `1` is `human-readable part` + `separator`, if we're splitting these up, maybe this would be a better documentation:
```suggestion
const auto hrp_size = 2, separator_size = 1;
if (input.size() + hrp_size + separator_size + bech32::CHECKSUM_SIZE <= bech32::CharLimit::BECH32) {
```

💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705377816)
nit: would it make sense to move these to the header as well - and make them static constexpr as well?
```suggestion
/** The Bech32 and Bech32m checksum size */
static constexpr size_t CHECKSUM_SIZE = 6;

/** The Bech32 and Bech32m character set for encoding. */
static constexpr const char* CHARSET = "qpzry9x8gf2tvdw0s3jn54khce6mua7l";

/** The Bech32 and Bech32m character set for decoding. */
static constexpr const int8_t CHARSET_REV[128] = {
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1
...

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1705382053)
👍, thanks for considering

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1705386853)
ok, the number part can make sense, I'm still not sure about the `Try` part - seems like https://en.wikipedia.org/wiki/Hungarian_notation, which (as a Hungarian) I'm very much against :p

> Return types are not immediately visible on in the callsite

It's part of the method's signature, I don't see how repeating it in the method's name helps.
The parameters also aren't visible on the call site, yet we're not encoding them in the method's name.

If you insist, I won't block you on this of
...

🤔 darosior reviewed a pull request: "descriptors: Be able to specify change and receiving in a single descriptor string"
(https://github.com/bitcoin/bitcoin/pull/22838#pullrequestreview-2220900524)
re-ACK 8208454eae7484d34a162ffa7701157e56e3cb80

Although i agree it's more readable now it's unfortunate to ask to rewrite part of the parsing logic only for style purpose after it had 3 ACKs and underwent significant fuzzing.

That said i didn't find any issue and neither did my fuzzer (for now). I'll keep fuzzing this PR rebased on master for a day.

💬 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#discussion_r1705294173)
If we are going to rewrite stuff just for style purpose, i'll reiterate that this search is quadratic in the number of multipath values. Inserting each value in an unordered set and checking against it instead would make it linear in the number of multipath values.

That said i don't think it matters for any real world usage and therefore we should keep it for a follow-up.

💬 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#discussion_r1705306602)
nit:
```patch
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 23faa78dac..7ef72bdda4 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1490,13 +1490,13 @@ std::optional<uint32_t> ParseKeyPathNum(Span<const char> elem, bool& apostrophe,
}

if (!multipath_segment_index) {
- out.emplace_back(path);
+ out.emplace_back(std::move(path));
} else {
// Replace the multipath placeholder with each value wh
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2271103348)
@paplorinc if you're not connecting to a local trusted node there will be too much variance in how quickly you can connect to peers and how reliably they will serve the blocks. Also, there will be barely any prune flushes the first 300k blocks anyways. That's a not a good benchmark for this.

💬 fanquake commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2271109782)
Backported for `27.x` in #30558.

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1705405872)
Got it, so our tests are run with a different version than what we're mentioning here, right?
Not sure it makes sense, but could we have one of the docker images have this version, to expose ourselves to the documented os version as well (e.g. to catch if someone uses an old dependency, like a `CMake 3.22.1` incompatibility)?

💬 naiyoma commented on pull request "cli: restrict multiple exclusive argument usage in bitcoin-cli":
(https://github.com/bitcoin/bitcoin/pull/30148#discussion_r1705412746)
thanks, added https://github.com/bitcoin/bitcoin/pull/30148/commits/8838c4f171f3c3e568d237b216167fddb521d0a7

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1705415336)
yeah (even edited the scripts to have them `ON` by default), I can run a benchmark from the terminal, but the IDE doesn't see them as part of the project.
Maybe @rot13maxi can confirm or deny whether CLion sees the benchmarks.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1705423262)
> Got it, so our tests are run with a different version than what we're mentioning here, right?

CMake version can be any >= 3.22. However, CMake ensures backward compatibility using the [policies](https://cmake.org/cmake/help/latest/manual/cmake-policies.7.html). Only policies known to version 3.22 are enabled, regardless of the actual CMake version.

> Not sure it makes sense, but could we have one of the docker images have this version, to expose ourselves to the documented os version as
...

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1705430340)
@paplorinc

Please open an issue in https://github.com/hebasto/bitcoin/issues with detailed steps to reproduce the problem from the scratch.

💬 josibake commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705432774)
Agree that fuzzing for more than just the hard-coded "bc" is ideal, but probably better as a separate follow-up.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2271150526)
> if you're not connecting to a local trusted node

This is by design. I want a real-world test. Others have already tested the local-node scenario, so I'll just work around the volatility by increasing the repetition.

> That's a not a good benchmark for this

I'm running 3 rounds now until 500k - that should be representative, right?

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1705436112)
Thanks

💬 josibake commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705436507)
Per https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705432774, it would be nice to not hardcode the HRP at all (or it's size) but I think it should be a separate and larger change. For the separator, I don't think it makes much difference if we use a variable or comment to mention what the value is for.

💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705439251)
I'm fine either way

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2271161893)
The CI reminds me of the recent Bitcoin price changes :p