💬 theStack commented on pull request "doc: update documentation and scripts related to build directories":
(https://github.com/bitcoin/bitcoin/pull/30741#issuecomment-2324908626)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30741#issuecomment-2324908626)
Concept ACK
💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2324917344)
`655a2cf666...99b1f88fe8`: rebase to pick CMake
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2324917344)
`655a2cf666...99b1f88fe8`: rebase to pick CMake
💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2324918318)
> add a test as a very first commit, which reproduces the old behavior first?
Yeah, I realize the test commit can't be cherry-picked cleanly to master. Updated to have the test commit first, and update the test for each change.
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2324918318)
> add a test as a very first commit, which reproduces the old behavior first?
Yeah, I realize the test commit can't be cherry-picked cleanly to master. Updated to have the test commit first, and update the test for each change.
💬 maflcko commented on pull request "refactor: Extend CScript with `operator<<` for `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741026359)
> > will this be touched again?
>
> Not sure, isn't that the case with vector as well?
Yes, so that is another reason that either the code should be either left as-is, (to avoid changing it now, and then again in the future), or to just change it once.
> > so it just seems like right now this is adding more code while leaving the underlying issue.
>
> Can you please detail why you think this is the case?
The goal of this change is to avoid a copy/conversion, however the change
...
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741026359)
> > will this be touched again?
>
> Not sure, isn't that the case with vector as well?
Yes, so that is another reason that either the code should be either left as-is, (to avoid changing it now, and then again in the future), or to just change it once.
> > so it just seems like right now this is adding more code while leaving the underlying issue.
>
> Can you please detail why you think this is the case?
The goal of this change is to avoid a copy/conversion, however the change
...
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2324928668)
`587d996f07...489c2477e7`: rebase to pick CMake
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2324928668)
`587d996f07...489c2477e7`: rebase to pick CMake
📝 maflcko opened a pull request: " bench: Remove redundant logging benchmarks "
(https://github.com/bitcoin/bitcoin/pull/30790)
`LogPrint*ThreadNames` is redundant with `LogWith(out)ThreadNames`,
because they all measure toggling the thread names (and check that it
has no effect on performance).
Fix it by removing the redundant ones. This also allows to drop a deprecated logging alias.
(https://github.com/bitcoin/bitcoin/pull/30790)
`LogPrint*ThreadNames` is redundant with `LogWith(out)ThreadNames`,
because they all measure toggling the thread names (and check that it
has no effect on performance).
Fix it by removing the redundant ones. This also allows to drop a deprecated logging alias.
💬 maflcko commented on pull request "fuzz: speed up addrman":
(https://github.com/bitcoin/bitcoin/pull/30688#discussion_r1741058041)
> I meant generating the size directly to be able to reserve, i.e.
Yes, I understood what you meant. My reply still holds. I don't see the benefit of creating possibly up to 1000 constant addresses when the fuzz input is exhausted. The bitdeque code you link to isn't a loop, so it looks unrelated. For a loop of up to 32, I can see how both ways are acceptable. And in the golumb_rice fuzz target the `ConsumeRandomLengthByteVector` may similarly return up to 512 empty vectors.
In practise, I
...
(https://github.com/bitcoin/bitcoin/pull/30688#discussion_r1741058041)
> I meant generating the size directly to be able to reserve, i.e.
Yes, I understood what you meant. My reply still holds. I don't see the benefit of creating possibly up to 1000 constant addresses when the fuzz input is exhausted. The bitdeque code you link to isn't a loop, so it looks unrelated. For a loop of up to 32, I can see how both ways are acceptable. And in the golumb_rice fuzz target the `ConsumeRandomLengthByteVector` may similarly return up to 512 empty vectors.
In practise, I
...
💬 l0rinc commented on pull request "refactor: Extend CScript with `operator<<` for `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741058744)
> however the change only fixes it for one very specific instance
Seems I missed some other cases, which other ones should we include here?
> leaving the problem otherwise unfixed
It does? This is what we've mentioned throughout the original PR by @hodlinator, @ryanofsky and yourself. I don't mind changing to span (it's what I suggested originally), if you think that would solve it in a cleaner way - but I need to understand the problem better since I though this fully solved our concer
...
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741058744)
> however the change only fixes it for one very specific instance
Seems I missed some other cases, which other ones should we include here?
> leaving the problem otherwise unfixed
It does? This is what we've mentioned throughout the original PR by @hodlinator, @ryanofsky and yourself. I don't mind changing to span (it's what I suggested originally), if you think that would solve it in a cleaner way - but I need to understand the problem better since I though this fully solved our concer
...
💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741063319)
> I presume you mean v.get_str()
No, I meant when it doesn't actually store a string, what should be the error.
> It appears to me that behaviour is unchanged
Yes, it's just that it's a hidden error which would be eliminated if `ParseHashV` accepted a `string_view` instead of a `UniValue`. I understand if it's outside the scope, just noting that there's another path that we might want to consider when evaluating the correctness of the method.
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741063319)
> I presume you mean v.get_str()
No, I meant when it doesn't actually store a string, what should be the error.
> It appears to me that behaviour is unchanged
Yes, it's just that it's a hidden error which would be eliminated if `ParseHashV` accepted a `string_view` instead of a `UniValue`. I understand if it's outside the scope, just noting that there's another path that we might want to consider when evaluating the correctness of the method.
💬 ryanofsky commented on pull request "refactor: Extend CScript with `operator<<` for `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741073162)
I think this should just take a span, not an array or a vector because the container type should not be relevant.
The only reason to not use span here would be to try to make the cscript << operator act like the serialization << operator and treat vectors differently from spans and arrays. This behavior makes some sense for serialization, but doesn't make sense for pushing cscript data, so I don't think should be a concern.
(For serialization we use Span{} casts as a way of telling seriali
...
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741073162)
I think this should just take a span, not an array or a vector because the container type should not be relevant.
The only reason to not use span here would be to try to make the cscript << operator act like the serialization << operator and treat vectors differently from spans and arrays. This behavior makes some sense for serialization, but doesn't make sense for pushing cscript data, so I don't think should be a concern.
(For serialization we use Span{} casts as a way of telling seriali
...
📝 hebasto opened a pull request: "build: Use correct variable name"
(https://github.com/bitcoin/bitcoin/pull/30791)
This was overlooked after https://github.com/bitcoin-core/secp256k1/pull/1546.
(https://github.com/bitcoin/bitcoin/pull/30791)
This was overlooked after https://github.com/bitcoin-core/secp256k1/pull/1546.
💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741075923)
> No, I meant when it doesn't actually store a string, what should be the error.
Yes, that's what the `v.get_str()` call checks internally, and it throws a `type_error` if so. Have a look t the `UniValue::get_str()` implementation? (Note that user input type validation happens in `RPCHelpMan::HandleRequest()`, but of course this can still happen through internal bugs).
> I understand if it's outside the scope
Yeah it's quite orthogonal to this PR, given the number of callsites of `Pars
...
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741075923)
> No, I meant when it doesn't actually store a string, what should be the error.
Yes, that's what the `v.get_str()` call checks internally, and it throws a `type_error` if so. Have a look t the `UniValue::get_str()` implementation? (Note that user input type validation happens in `RPCHelpMan::HandleRequest()`, but of course this can still happen through internal bugs).
> I understand if it's outside the scope
Yeah it's quite orthogonal to this PR, given the number of callsites of `Pars
...
💬 sipa commented on pull request "refactor: Extend CScript with `operator<<` for `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741078658)
Would it make sense to drop operator<< from CScript, and instead have explicit "Append" and "Push" member functions (that take spans), and do the expected thing?
(I have not followed this PR in detail, feel free to ignore if I'm missing things)
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741078658)
Would it make sense to drop operator<< from CScript, and instead have explicit "Append" and "Push" member functions (that take spans), and do the expected thing?
(I have not followed this PR in detail, feel free to ignore if I'm missing things)
💬 ryanofsky commented on pull request "refactor: Extend CScript with `operator<<` for `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741079145)
It would also be great if this could be more clearly documented as pushing data as marco suggests, and if it could use std::byte instead of char. If using std::byte is not easily possible, maybe it could use `BasicByte` so the PR can change `_hex_v_u8` to `_hex` instead of `_hex_u8`
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741079145)
It would also be great if this could be more clearly documented as pushing data as marco suggests, and if it could use std::byte instead of char. If using std::byte is not easily possible, maybe it could use `BasicByte` so the PR can change `_hex_v_u8` to `_hex` instead of `_hex_u8`
💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741088168)
> Have a look at the UniValue::get_str() implementation
I already checked all of that, that's why I was asking if we'd rather they throw on the call site. You've already answered that you think it's orthogonal.
> I am generally in favour of pushing UniValue user input parsing into stricter types further back to the edges, though.
Yes, that's what I was saying basically - though I needed 2 rounds to understand it myself why this was bothering me.
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741088168)
> Have a look at the UniValue::get_str() implementation
I already checked all of that, that's why I was asking if we'd rather they throw on the call site. You've already answered that you think it's orthogonal.
> I am generally in favour of pushing UniValue user input parsing into stricter types further back to the edges, though.
Yes, that's what I was saying basically - though I needed 2 rounds to understand it myself why this was bothering me.
👍 l0rinc approved a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2275917562)
Minor nit suggestions, ACK a9d19ae1aa06f14c1892bb4e37c3f023bc61b39b
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2275917562)
Minor nit suggestions, ACK a9d19ae1aa06f14c1892bb4e37c3f023bc61b39b
💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741081872)
since we've already included `setup_common` which contains `std::ostream& operator<<(std::ostream& os, const arith_uint256& num)` we might as well use `BOOST_CHECK_EQUAL` here:
```diff
diff --git a/src/test/arith_uint256_tests.cpp b/src/test/arith_uint256_tests.cpp
index a434a1d401..9a6307c1bd 100644
--- a/src/test/arith_uint256_tests.cpp
+++ b/src/test/arith_uint256_tests.cpp
@@ -64,51 +64,50 @@ static std::string ArrayToString(const unsigned char A[], unsigned int width)
BOOST_AUTO_
...
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741081872)
since we've already included `setup_common` which contains `std::ostream& operator<<(std::ostream& os, const arith_uint256& num)` we might as well use `BOOST_CHECK_EQUAL` here:
```diff
diff --git a/src/test/arith_uint256_tests.cpp b/src/test/arith_uint256_tests.cpp
index a434a1d401..9a6307c1bd 100644
--- a/src/test/arith_uint256_tests.cpp
+++ b/src/test/arith_uint256_tests.cpp
@@ -64,51 +64,50 @@ static std::string ArrayToString(const unsigned char A[], unsigned int width)
BOOST_AUTO_
...
💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741091951)
we don't have to add a variable, we can just inline it to:
```C++
if (fDoFullFlush || m_next_write == NodeClock::time_point::max()) {
```
Which would simplify a lot (logically, making it obvious when we're queuing the next run and also codewise, reuding duplication)
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741091951)
we don't have to add a variable, we can just inline it to:
```C++
if (fDoFullFlush || m_next_write == NodeClock::time_point::max()) {
```
Which would simplify a lot (logically, making it obvious when we're queuing the next run and also codewise, reuding duplication)
💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741092396)
Mostly because I ran it and it behaved in a weird way (sometimes passing) so I have a hard time trusting it now. It never passed for ALL of these values, so at least they're more reliable.
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741092396)
Mostly because I ran it and it behaved in a weird way (sometimes passing) so I have a hard time trusting it now. It never passed for ALL of these values, so at least they're more reliable.
💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1741093949)
done
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1741093949)
done