💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682051928)
I haven't been able to come up with something that I think would be an improvement. I did split the second last commit up into two commits though. Maybe that will make it more clear when reviewing.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682051928)
I haven't been able to come up with something that I think would be an improvement. I did split the second last commit up into two commits though. Maybe that will make it more clear when reviewing.
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682053641)
You might be right here, but this function in particular is very heavily commented due to its importance. I think modifying the comment is better than just removing it.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682053641)
You might be right here, but this function in particular is very heavily commented due to its importance. I think modifying the comment is better than just removing it.
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1682053666)
Hi @hodlinator do you know if there is something I can do to get this merged?
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1682053666)
Hi @hodlinator do you know if there is something I can do to get this merged?
💬 ajtowns commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682113718)
Should be fixed (line is no longer touched)
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682113718)
Should be fixed (line is no longer touched)
💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1682264742)
Don't know of a good way unfortunately. Maybe one of the other reviewers will react soon from our comment activity today.
Did a quick search for `HelpExampleRpc` now and there are a few other RPCs which have quite a few examples.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1682264742)
Don't know of a good way unfortunately. Maybe one of the other reviewers will react soon from our comment activity today.
Did a quick search for `HelpExampleRpc` now and there are a few other RPCs which have quite a few examples.
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682274787)
Not sure the latest push is a good change. I don't think this should be replaced with a constructor in the future, but it should be removed completely.
A new function (for example `static std::optional<base_blob<N>> base_blob<N>::FromHex(std::string_view)` can be added that parses and then returns an `std::optional`. Similar to TryParseHex.
In any case, the doxygen comment here is probably the wrong place to track brainstorming issues.
If we really wanted to turn this into a constructor
...
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682274787)
Not sure the latest push is a good change. I don't think this should be replaced with a constructor in the future, but it should be removed completely.
A new function (for example `static std::optional<base_blob<N>> base_blob<N>::FromHex(std::string_view)` can be added that parses and then returns an `std::optional`. Similar to TryParseHex.
In any case, the doxygen comment here is probably the wrong place to track brainstorming issues.
If we really wanted to turn this into a constructor
...
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682324270)
Tried:
```diff
diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp
index 2053a42d24..e8dfb03d18 100644
--- a/src/test/uint256_tests.cpp
+++ b/src/test/uint256_tests.cpp
@@ -119,6 +119,10 @@ BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality
BOOST_CHECK(uint160(R1S) == R1S);
BOOST_CHECK(uint160(ZeroS) == ZeroS);
BOOST_CHECK(uint160(OneS) == OneS);
+
+ uint256{0};
+ uint256{NULL};
+ uint256{nullptr};
}
BOOST_AUTO_TEST_C
...
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682324270)
Tried:
```diff
diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp
index 2053a42d24..e8dfb03d18 100644
--- a/src/test/uint256_tests.cpp
+++ b/src/test/uint256_tests.cpp
@@ -119,6 +119,10 @@ BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality
BOOST_CHECK(uint160(R1S) == R1S);
BOOST_CHECK(uint160(ZeroS) == ZeroS);
BOOST_CHECK(uint160(OneS) == OneS);
+
+ uint256{0};
+ uint256{NULL};
+ uint256{nullptr};
}
BOOST_AUTO_TEST_C
...
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682344482)
> Does CI have a stricter warnings-as-errors policy?
Yeah, IIRC it should treat warnings as error.
Even if it didn't, the compile warning and `nullptr` in code would still have to pass the eyes of the developer and reviewers. Also, the CI would fail later on with a segfault anyway.
As for the others:
* `0` is intentional and used in code today, see:
```
const uint256 uint256::ZERO(0);
const uint256 uint256::ONE(1);
```
* NULL is forbidden by clang-tidy, see `modernize-use-null
...
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682344482)
> Does CI have a stricter warnings-as-errors policy?
Yeah, IIRC it should treat warnings as error.
Even if it didn't, the compile warning and `nullptr` in code would still have to pass the eyes of the developer and reviewers. Also, the CI would fail later on with a segfault anyway.
As for the others:
* `0` is intentional and used in code today, see:
```
const uint256 uint256::ZERO(0);
const uint256 uint256::ONE(1);
```
* NULL is forbidden by clang-tidy, see `modernize-use-null
...
💬 dergoegge commented on pull request "fuzz: Limit parse_univalue input length":
(https://github.com/bitcoin/bitcoin/pull/30473#issuecomment-2235821583)
Could you elaborate what exactly is making this input so slow? I doubt it's the actual json parsing. It looks more like another descriptor parsing "bug".
(https://github.com/bitcoin/bitcoin/pull/30473#issuecomment-2235821583)
Could you elaborate what exactly is making this input so slow? I doubt it's the actual json parsing. It looks more like another descriptor parsing "bug".
💬 maflcko commented on pull request "fuzz: Limit parse_univalue input length":
(https://github.com/bitcoin/bitcoin/pull/30473#issuecomment-2235826082)
Correct, the workaround for the descriptor parsing are not applied here. So an alternative would be to apply them here.
(https://github.com/bitcoin/bitcoin/pull/30473#issuecomment-2235826082)
Correct, the workaround for the descriptor parsing are not applied here. So an alternative would be to apply them here.
💬 maflcko commented on pull request "fuzz: Limit parse_univalue input length":
(https://github.com/bitcoin/bitcoin/pull/30473#issuecomment-2235830888)
With "workaround" I mean https://github.com/bitcoin/bitcoin/pull/30197
(https://github.com/bitcoin/bitcoin/pull/30473#issuecomment-2235830888)
With "workaround" I mean https://github.com/bitcoin/bitcoin/pull/30197
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682359897)
Reset back to prior version. As you say it seems we could convert `uint256S()` to a constructor already thanks to the pre-C++23 warnings by Clang & GCC, but I think that is outside the scope of this PR.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682359897)
Reset back to prior version. As you say it seems we could convert `uint256S()` to a constructor already thanks to the pre-C++23 warnings by Clang & GCC, but I think that is outside the scope of this PR.
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682361588)
Reset back to prior version, see https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682274787.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682361588)
Reset back to prior version, see https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682274787.
💬 dergoegge commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682362271)
Perhaps a more descriptive comment would be good, e.g.: `We don't call PickValue on the outpoints set directly because PickValue will advance a begin iterator on the set, which is expensive, because it only has a "++" operator. As it would be called in a loop of num_in (~outpoints.size()), the runtime would be O(outpoints.size() ^ 2). Instead we create a std::vector copy of the set, which allows for O(1) std::advance.`.
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682362271)
Perhaps a more descriptive comment would be good, e.g.: `We don't call PickValue on the outpoints set directly because PickValue will advance a begin iterator on the set, which is expensive, because it only has a "++" operator. As it would be called in a loop of num_in (~outpoints.size()), the runtime would be O(outpoints.size() ^ 2). Instead we create a std::vector copy of the set, which allows for O(1) std::advance.`.
💬 maflcko commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#issuecomment-2235847045)
I forgot to mention the fuzz input for testing. It is `c6efad2b3a41cf2a2a682dabed1310bf5e3c101e`. See https://cirrus-ci.com/task/6178647134961664?logs=ci#L4058
```
Run txorphan with args ['/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/txorphan')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3180047797
INFO: Loaded 1 modules (623498 inline 8-bit cou
...
(https://github.com/bitcoin/bitcoin/pull/30474#issuecomment-2235847045)
I forgot to mention the fuzz input for testing. It is `c6efad2b3a41cf2a2a682dabed1310bf5e3c101e`. See https://cirrus-ci.com/task/6178647134961664?logs=ci#L4058
```
Run txorphan with args ['/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/txorphan')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3180047797
INFO: Loaded 1 modules (623498 inline 8-bit cou
...
💬 maflcko commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682373792)
Thanks, done!
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682373792)
Thanks, done!
✅ maflcko closed an issue: "fuzz: Fix timeouts"
(https://github.com/bitcoin/bitcoin/issues/28812)
(https://github.com/bitcoin/bitcoin/issues/28812)
💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-2235880636)
Closing for now. It is probably better to create separate issues for each fuzz target, going forward.
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-2235880636)
Closing for now. It is probably better to create separate issues for each fuzz target, going forward.
💬 craigraw commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2235889412)
> Would it not be possible to implement this by creating a wallet called broadcast_pool and then write simple wrapper functions to implement the broadcast pool features?
All of the wallets linked in the issues above (and of course many others) use the Electrum server protocol, and in general rely on Electrum server implementations that do not require or use the wallet functionality. Even if they were to do so, I cannot see how it is practical to broadcast arbitrary transactions via the wallet
...
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2235889412)
> Would it not be possible to implement this by creating a wallet called broadcast_pool and then write simple wrapper functions to implement the broadcast pool features?
All of the wallets linked in the issues above (and of course many others) use the Electrum server protocol, and in general rely on Electrum server implementations that do not require or use the wallet functionality. Even if they were to do so, I cannot see how it is practical to broadcast arbitrary transactions via the wallet
...
⚠️ maflcko opened an issue: "scriptpubkeyman fuzz target TopUp is slow"
(https://github.com/bitcoin/bitcoin/issues/30476)
Seems wasteful to spend time in `TopUp` while fuzzing, so my recommendation would be to limit TopUp iterations a bit.
Context: https://cirrus-ci.com/task/6178647134961664?logs=ci#L4357
```
Run scriptpubkeyman with args ['/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/scriptpubkeyman')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3840853548
INFO: Loa
...
(https://github.com/bitcoin/bitcoin/issues/30476)
Seems wasteful to spend time in `TopUp` while fuzzing, so my recommendation would be to limit TopUp iterations a bit.
Context: https://cirrus-ci.com/task/6178647134961664?logs=ci#L4357
```
Run scriptpubkeyman with args ['/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/scriptpubkeyman')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3840853548
INFO: Loa
...