💬 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
...
(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.
(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.
(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)?
(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
(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.
(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
...
(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.
(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.
(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?
(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
(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.
(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
(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
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2271161893)
The CI reminds me of the recent Bitcoin price changes :p
💬 glozow commented on pull request "doc, rpc : `#30275` followups":
(https://github.com/bitcoin/bitcoin/pull/30525#discussion_r1705470107)
I think the previous text is clear and not inaccurate, so I'm wondering whether this last commit is necessary.
(https://github.com/bitcoin/bitcoin/pull/30525#discussion_r1705470107)
I think the previous text is clear and not inaccurate, so I'm wondering whether this last commit is necessary.
💬 darosior commented on issue "Unexpected behaviour when using `sortedmulti_a` descriptor":
(https://github.com/bitcoin/bitcoin/issues/30518#issuecomment-2271210770)
I'm not sure. `sortedmulti_a` was introduced for compatibility with legacy pre-descriptor practices. I can't think of any reason to ever prefer `sortedmulti_a`/`sortedmulti` to `multi_a`/`multi` inside Miniscript. And there is no legacy compatibility concern since any Miniscript-using wallet must have been created in a post-descriptor world.
So i don't think it's worth adding `sorted*` fragments to Miniscript.
(https://github.com/bitcoin/bitcoin/issues/30518#issuecomment-2271210770)
I'm not sure. `sortedmulti_a` was introduced for compatibility with legacy pre-descriptor practices. I can't think of any reason to ever prefer `sortedmulti_a`/`sortedmulti` to `multi_a`/`multi` inside Miniscript. And there is no legacy compatibility concern since any Miniscript-using wallet must have been created in a post-descriptor world.
So i don't think it's worth adding `sorted*` fragments to Miniscript.
👍 TheCharlatan approved a pull request: "build: Remove unused visibility checks"
(https://github.com/bitcoin/bitcoin/pull/30590#pullrequestreview-2221207346)
ACK bbcba09cd5ca5fdd9055aaf64781125c5e505576
The `visibility("default")` attribute is still used in `src/tinyformat.h`, but I don't think we need this check for that, since it is already gated.
(https://github.com/bitcoin/bitcoin/pull/30590#pullrequestreview-2221207346)
ACK bbcba09cd5ca5fdd9055aaf64781125c5e505576
The `visibility("default")` attribute is still used in `src/tinyformat.h`, but I don't think we need this check for that, since it is already gated.
💬 josibake commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705483297)
IIUC, I don't think static makes a difference here since we aren't talking about a class member? Regarding `CHARSET` and `CHARSET_REV`, what's the advantage of having them in the header file if we aren't using them outside of bech32.cpp?
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705483297)
IIUC, I don't think static makes a difference here since we aren't talking about a class member? Regarding `CHARSET` and `CHARSET_REV`, what's the advantage of having them in the header file if we aren't using them outside of bech32.cpp?
💬 fanquake commented on pull request "build: Remove unused visibility checks":
(https://github.com/bitcoin/bitcoin/pull/30590#issuecomment-2271230120)
> but I don't think we need this check for that, since it is already gated.
Yea. I think for our use, that (and related) code could also just be removed entirely.
(https://github.com/bitcoin/bitcoin/pull/30590#issuecomment-2271230120)
> but I don't think we need this check for that, since it is already gated.
Yea. I think for our use, that (and related) code could also just be removed entirely.
💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705520136)
> I don't think static makes a difference here
Based on other usages in the code, `static constexpr` in headers is good practice to prevent potential linking issues and ensure each translation unit has its own independent copy. Let me know if I'm wrong here.
> what's the advantage of having them in the header file
Separating constants and definitions into the header, leaving implementations in the cpp, i.e. separation of concerns.
If you think this is outside the scope, I can accept
...
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705520136)
> I don't think static makes a difference here
Based on other usages in the code, `static constexpr` in headers is good practice to prevent potential linking issues and ensure each translation unit has its own independent copy. Let me know if I'm wrong here.
> what's the advantage of having them in the header file
Separating constants and definitions into the header, leaving implementations in the cpp, i.e. separation of concerns.
If you think this is outside the scope, I can accept
...