💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#issuecomment-2324882761)
> it seems odd that the fuzz tests would time out here
Split the big fuzz target into separate base58_encode_decode/base58check_encode_decode/base32_encode_decode/base64_encode_decode/psbt_base64_decode - it's cleaner like this anyway.
(https://github.com/bitcoin/bitcoin/pull/30746#issuecomment-2324882761)
> it seems odd that the fuzz tests would time out here
Split the big fuzz target into separate base58_encode_decode/base58check_encode_decode/base32_encode_decode/base64_encode_decode/psbt_base64_decode - it's cleaner like this anyway.
📝 theStack opened a pull request: "scripted-diff: adapt binary paths in docs (`./src/...` -> `./build/src/...`)"
(https://github.com/bitcoin/bitcoin/pull/30789)
This is a simple quick-fix to make the docs match the new directory structure after the switch to CMake (see #30454 and #30664). On the long term, it may make sense to improve some of these docs by introducing and using environment variables like `BITCOIN_CLI` and `BITCOIND` instead of specifying the path repeatedly.
(https://github.com/bitcoin/bitcoin/pull/30789)
This is a simple quick-fix to make the docs match the new directory structure after the switch to CMake (see #30454 and #30664). On the long term, it may make sense to improve some of these docs by introducing and using environment variables like `BITCOIN_CLI` and `BITCOIND` instead of specifying the path repeatedly.
💬 fanquake commented on pull request "scripted-diff: adapt binary paths in docs (`./src/...` -> `./build/src/...`)":
(https://github.com/bitcoin/bitcoin/pull/30789#issuecomment-2324898009)
Note this is also included in #30741.
(https://github.com/bitcoin/bitcoin/pull/30789#issuecomment-2324898009)
Note this is also included in #30741.
💬 l0rinc commented on pull request "refactor: Extend CScript with `operator<<` for `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741014685)
> will this be touched again?
Not sure, isn't that the case with vector as well?
> 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? `_hex_v_u8` was mostly eliminated and the production code is basically the same (+3 lines for the other overload).
> Maybe this could be a span or a range?
I understood we wanted to avoid doing that in https://github.com/bitcoin/bitcoin/pull/30377#discus
...
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741014685)
> will this be touched again?
Not sure, isn't that the case with vector as well?
> 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? `_hex_v_u8` was mostly eliminated and the production code is basically the same (+3 lines for the other overload).
> Maybe this could be a span or a range?
I understood we wanted to avoid doing that in https://github.com/bitcoin/bitcoin/pull/30377#discus
...
💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#issuecomment-2324906247)
And force-pushed again to rebase on top of #30377 to avoid silent merge conflicts e.g. because of the now lowercase-only `uint256` hex string constructor.
(https://github.com/bitcoin/bitcoin/pull/30773#issuecomment-2324906247)
And force-pushed again to rebase on top of #30377 to avoid silent merge conflicts e.g. because of the now lowercase-only `uint256` hex string constructor.
✅ theStack closed a pull request: "scripted-diff: adapt binary paths in docs (`./src/...` -> `./build/src/...`)"
(https://github.com/bitcoin/bitcoin/pull/30789)
(https://github.com/bitcoin/bitcoin/pull/30789)
💬 theStack commented on pull request "scripted-diff: adapt binary paths in docs (`./src/...` -> `./build/src/...`)":
(https://github.com/bitcoin/bitcoin/pull/30789#issuecomment-2324907676)
> Note this is also included in #30741.
Oh, I missed that one, it seems to be more thorough anyways (e.g. also tackling .bt files). Closing in favor of #30741.
(https://github.com/bitcoin/bitcoin/pull/30789#issuecomment-2324907676)
> Note this is also included in #30741.
Oh, I missed that one, it seems to be more thorough anyways (e.g. also tackling .bt files). Closing in favor of #30741.
💬 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)