💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740999675)
Done.
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740999675)
Done.
💬 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
...