👍 tdb3 approved a pull request: "test: Non-Shy version sender"
(https://github.com/bitcoin/bitcoin/pull/30453#pullrequestreview-2184376970)
ACK faed5d3870fb32fb5278961ee23e38fd9ea6ca15

Test causes framework to be "non-shy"


💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682048700)
Split into two commits.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682050138)
Done.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682050222)
Done.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682050453)
Removed.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682050701)
Updated the comment to @sipa's version.

💬 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.

💬 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.

💬 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?

💬 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)

💬 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.

💬 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
...

💬 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
...

💬 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
...

💬 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".

💬 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.

💬 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

💬 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.

💬 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.

💬 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.`.