💬 garlonicon commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2329009795)
I think there is a small risk, that we will see a huge double-spending attempt in testnet4, because after block 00000000000000263393ce5f648afd53676f13d360cc9f264b89351623bf1242, there are transactions in the old chain, which are not picked by nodes, mining the new chain. Which means, that suddenly, when the deep reorg will revert hundreds of blocks, all of those transactions will suddenly become unconfirmed.
So, if anyone has any coins, which were received after block number 42335, then they
...
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2329009795)
I think there is a small risk, that we will see a huge double-spending attempt in testnet4, because after block 00000000000000263393ce5f648afd53676f13d360cc9f264b89351623bf1242, there are transactions in the old chain, which are not picked by nodes, mining the new chain. Which means, that suddenly, when the deep reorg will revert hundreds of blocks, all of those transactions will suddenly become unconfirmed.
So, if anyone has any coins, which were received after block number 42335, then they
...
💬 sdaftuar commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2329027542)
> @sdaftuar do you think you could instrument your code to gather/dump clusters that linearize optimally with say 1,000,000 iterations but not using 100,000 iterations, using the code up to commit [21b826a](https://github.com/bitcoin/bitcoin/commit/21b826af07fdfa26aaf981cc0810edd6b55b45e0) (before that commit the search algorithm is so naive that high iteration count isn't really an indication of being hard).
I used my simulation environment to run the commit you listed on last year's data, s
...
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2329027542)
> @sdaftuar do you think you could instrument your code to gather/dump clusters that linearize optimally with say 1,000,000 iterations but not using 100,000 iterations, using the code up to commit [21b826a](https://github.com/bitcoin/bitcoin/commit/21b826af07fdfa26aaf981cc0810edd6b55b45e0) (before that commit the search algorithm is so naive that high iteration count isn't really an indication of being hard).
I used my simulation environment to run the commit you listed on last year's data, s
...
💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743693696)
style nit in 95b3aca0eb3fc49cd99b8de028a676bb04e2157f: Could have the happy-case early-return in the second line of the function, like in a prior version of this pull request.
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743693696)
style nit in 95b3aca0eb3fc49cd99b8de028a676bb04e2157f: Could have the happy-case early-return in the second line of the function, like in a prior version of this pull request.
🤔 maflcko reviewed a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2280021001)
Concept ACK.
I left another question in the second commit. Sorry for the incremental review.
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2280021001)
Concept ACK.
I left another question in the second commit. Sorry for the incremental review.
💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743729122)
question in 948bb07715f9ef1d90b91dfbea4f487f755498f5: Is there a reason why the tests are dropped? I read the commit message and I understand that the comment says "String constructors", but I think it meant to say `UintToArith256 testing from a string`, which is not a test-only function.
This is my fault, because I forgot to update the comment in commit facf629ce8ff1d1f6d254dde4e89b5018f8df60e.
I also understand that there is a `BOOST_AUTO_TEST_CASE(conversion)` test. However, I think it
...
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743729122)
question in 948bb07715f9ef1d90b91dfbea4f487f755498f5: Is there a reason why the tests are dropped? I read the commit message and I understand that the comment says "String constructors", but I think it meant to say `UintToArith256 testing from a string`, which is not a test-only function.
This is my fault, because I forgot to update the comment in commit facf629ce8ff1d1f6d254dde4e89b5018f8df60e.
I also understand that there is a `BOOST_AUTO_TEST_CASE(conversion)` test. However, I think it
...
💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743786908)
Sorry for misunderstanding, but I think an error message of
`"txid must be string of 64 hex digits (not '1d999634f709c98f242f2e553bf0fdac7deb6f972c44c8c1b4d576d564acc7f')"` is unclear whether the error is that there are non-hex digits or whether there are missing or extraneous characters. And it is not trivial to see from looking at it.
A proposed error message of `"txid must be string of 64 hex digits (not '1d999634f709c98f242f2e553bf0fdac7deb6f972c44c8c1b4d576d564acc7f' of len $num)"` wh
...
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743786908)
Sorry for misunderstanding, but I think an error message of
`"txid must be string of 64 hex digits (not '1d999634f709c98f242f2e553bf0fdac7deb6f972c44c8c1b4d576d564acc7f')"` is unclear whether the error is that there are non-hex digits or whether there are missing or extraneous characters. And it is not trivial to see from looking at it.
A proposed error message of `"txid must be string of 64 hex digits (not '1d999634f709c98f242f2e553bf0fdac7deb6f972c44c8c1b4d576d564acc7f' of len $num)"` wh
...
💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743805522)
I tend to agree that the length is superfluous (we're usually not *that* user friendly with our error messages) and in case of error the user would likely copy-paste again which would likely solve the error.
I'm fine with both, just don't revert anything @stickies-v :)
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743805522)
I tend to agree that the length is superfluous (we're usually not *that* user friendly with our error messages) and in case of error the user would likely copy-paste again which would likely solve the error.
I'm fine with both, just don't revert anything @stickies-v :)
💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743810469)
Some of these were reverted since it seemed to us that it's testing prefixes and whitespaces primarily, see https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1740942391
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743810469)
Some of these were reverted since it seemed to us that it's testing prefixes and whitespaces primarily, see https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1740942391
💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743817035)
I agree the prefix and whitespace testing is useless and the prefix and whitespace can simply be removed in the test cases, but it is not trivially clear to me that the `UintToArith256` tests can be removed.
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743817035)
I agree the prefix and whitespace testing is useless and the prefix and whitespace can simply be removed in the test cases, but it is not trivially clear to me that the `UintToArith256` tests can be removed.
💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743824670)
Ah yes that would be better, will update this in next force-push, thanks!
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743824670)
Ah yes that would be better, will update this in next force-push, thanks!
💬 maflcko commented on pull request "test: add check that too large txs aren't put into orphanage":
(https://github.com/bitcoin/bitcoin/pull/30784#issuecomment-2329110687)
review ACK 66d13c870284327abc89d36c0b5cc5f58e96f570
(https://github.com/bitcoin/bitcoin/pull/30784#issuecomment-2329110687)
review ACK 66d13c870284327abc89d36c0b5cc5f58e96f570
💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#issuecomment-2329122240)
ACK da4377dc2ad0f495ebb97722d6cc2a95850363fa
I didn't mind the previous version either, but maybe this preserves more of what's not strictly related to the PR.
I think it's an improvement as-is, but I will gladly reack if you decide to include the other reviews as well.
(https://github.com/bitcoin/bitcoin/pull/30773#issuecomment-2329122240)
ACK da4377dc2ad0f495ebb97722d6cc2a95850363fa
I didn't mind the previous version either, but maybe this preserves more of what's not strictly related to the PR.
I think it's an improvement as-is, but I will gladly reack if you decide to include the other reviews as well.
💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743824684)
Nit1: consider narrowing the scope of expected:
Nit2: usually we compare the actual against the expected (switched the order):
```suggestion
if (auto expected_len{uint256::size() * 2}; strHex.length() != expected_len) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", name, expected_len, strHex.length(), strHex));
}
```
No strong feelings either way, feel free to ignore if you don't like it
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743824684)
Nit1: consider narrowing the scope of expected:
Nit2: usually we compare the actual against the expected (switched the order):
```suggestion
if (auto expected_len{uint256::size() * 2}; strHex.length() != expected_len) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", name, expected_len, strHex.length(), strHex));
}
```
No strong feelings either way, feel free to ignore if you don't like it
💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743835345)
> is not trivially clear to me that the UintToArith256 tests can be removed
+1 for restoring them in that case
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743835345)
> is not trivially clear to me that the UintToArith256 tests can be removed
+1 for restoring them in that case
💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743832221)
You mean something like:
```C++
uint256 ParseHashV(const UniValue& v, std::string_view name)
{
const std::string& strHex(v.get_str());
if (auto rv{uint256::FromHex(strHex)}){
return *rv;
} else if (auto expected_len{uint256::size() * 2}; strHex.length() != expected_len) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", name, expected_len, strHex.length(), strHex));
} else {
throw JSONRPCError(RPC_
...
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743832221)
You mean something like:
```C++
uint256 ParseHashV(const UniValue& v, std::string_view name)
{
const std::string& strHex(v.get_str());
if (auto rv{uint256::FromHex(strHex)}){
return *rv;
} else if (auto expected_len{uint256::size() * 2}; strHex.length() != expected_len) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", name, expected_len, strHex.length(), strHex));
} else {
throw JSONRPCError(RPC_
...
💬 fanquake commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1743843129)
From https://github.com/bitcoin/bitcoin/actions/runs/10665138943/job/29557888413#step:8:1:
```bash
Preset CMake variables:
BUILD_GUI="ON"
VCPKG_TARGET_TRIPLET="x64-windows-static"
WITH_NATPMP="OFF"
WITH_QRENCODE="OFF"
```
Can you explain why some options are passed on the command line, and why some are put into `CMakePresets.json`; what determines where each option belongs? I'd think it'd be better to have a single source of truth for the CI config.
Also, I see [compared to 28
...
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1743843129)
From https://github.com/bitcoin/bitcoin/actions/runs/10665138943/job/29557888413#step:8:1:
```bash
Preset CMake variables:
BUILD_GUI="ON"
VCPKG_TARGET_TRIPLET="x64-windows-static"
WITH_NATPMP="OFF"
WITH_QRENCODE="OFF"
```
Can you explain why some options are passed on the command line, and why some are put into `CMakePresets.json`; what determines where each option belongs? I'd think it'd be better to have a single source of truth for the CI config.
Also, I see [compared to 28
...
💬 fjahr commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743843312)
Yeah, much simpler, thanks, done!
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743843312)
Yeah, much simpler, thanks, done!
💬 fjahr commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743843443)
done
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743843443)
done
💬 fjahr commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743843548)
done
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743843548)
done
💬 fanquake commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1743850784)
For example, I can imagine someone changes `CMakePresets.json`, without realising that also partially controls the CI, and then we (silently) lose Qt compilation in this build.
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1743850784)
For example, I can imagine someone changes `CMakePresets.json`, without realising that also partially controls the CI, and then we (silently) lose Qt compilation in this build.
💬 maflcko commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743855915)
Forgot the CBlockIndex `*` -> `&` ? :sweat_smile:
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743855915)
Forgot the CBlockIndex `*` -> `&` ? :sweat_smile: