💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742071054)
nit: missing `optional` and `ostream` includes
<details>
<summary>iwyu</summary>
```
/ci_container_base/src/test/util/setup_common.h should add these lines:
#include <stddef.h> // for size_t
#include <stdint.h> // for uint32_t
#include <exception> // for exception
#include <memory> // for make_unique, unique_ptr
#include <optional> // for optional
#include <ostream> // for ostrea
...
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742071054)
nit: missing `optional` and `ostream` includes
<details>
<summary>iwyu</summary>
```
/ci_container_base/src/test/util/setup_common.h should add these lines:
#include <stddef.h> // for size_t
#include <stdint.h> // for uint32_t
#include <exception> // for exception
#include <memory> // for make_unique, unique_ptr
#include <optional> // for optional
#include <ostream> // for ostrea
...
💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742093199)
e0a96acc82c2c918572a2c9b90431db5bca19174: I don't really see the purpose of this ~move-only change and would rather this file can be dropped from the diff entirely (the include updates feel rather arbitrary so I'm okay with them being dropped too)
I don't think moving these functions is particularly helpful in the header either but can somewhat see the rationale since you're adding new code to it (still prefer it wouldn't move, though), but here it just feels like unnecessary churn?
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742093199)
e0a96acc82c2c918572a2c9b90431db5bca19174: I don't really see the purpose of this ~move-only change and would rather this file can be dropped from the diff entirely (the include updates feel rather arbitrary so I'm okay with them being dropped too)
I don't think moving these functions is particularly helpful in the header either but can somewhat see the rationale since you're adding new code to it (still prefer it wouldn't move, though), but here it just feels like unnecessary churn?
💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742149892)
nit: could use the consteval `uint256` hex string ctor here (+ below)
```suggestion
BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0x1234"}).assumed_valid_block, uint256{"0000000000000000000000000000000000000000000000000000000000001234"});
```
<details>
<summary>git diff on f509948e1e</summary>
```diff
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 3914deb926..e9d9fee1fe 100644
--- a/src/test/validation
...
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742149892)
nit: could use the consteval `uint256` hex string ctor here (+ below)
```suggestion
BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0x1234"}).assumed_valid_block, uint256{"0000000000000000000000000000000000000000000000000000000000001234"});
```
<details>
<summary>git diff on f509948e1e</summary>
```diff
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 3914deb926..e9d9fee1fe 100644
--- a/src/test/validation
...
💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742175853)
I'm not sure these 3 lines are worth adding, since they're already covered [higher up](https://github.com/bitcoin/bitcoin/pull/30618/files#diff-12dd31b274739b6a132d9c57e5ab14b403436991e4201cf8d3f3cc4fc7995066R29-R34), so this might unnecessarily slow down the test?
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742175853)
I'm not sure these 3 lines are worth adding, since they're already covered [higher up](https://github.com/bitcoin/bitcoin/pull/30618/files#diff-12dd31b274739b6a132d9c57e5ab14b403436991e4201cf8d3f3cc4fc7995066R29-R34), so this might unnecessarily slow down the test?
🤔 glozow reviewed a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2277710418)
Concept ACK
I once talked with @0xB10C about adding this, but never got around to it. I think it would be helpful for visualizing / trying out different orphan protection methods.
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2277710418)
Concept ACK
I once talked with @0xB10C about adding this, but never got around to it. I think it would be helpful for visualizing / trying out different orphan protection methods.
💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1742222919)
The orphanage can contain duplicates by txid, so adding wtxid as well could be helpful. This could be a `UniValue::VOBJ` instead, with both fields? That would allow adding more fields in the future (perhaps with different verbosity levels), e.g. telling us the size or originator.
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1742222919)
The orphanage can contain duplicates by txid, so adding wtxid as well could be helpful. This could be a `UniValue::VOBJ` instead, with both fields? That would allow adding more fields in the future (perhaps with different verbosity levels), e.g. telling us the size or originator.
💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1742224059)
What's wrong with a `std::vector<CTransactionRef>`? Doesn't this need to make copies of the `CTransaction` objects?
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1742224059)
What's wrong with a `std::vector<CTransactionRef>`? Doesn't this need to make copies of the `CTransaction` objects?
💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1742232560)
Previously we disambiguated between length errors and other errors. Since that's not longer true, I think not including the length in the error is the most elegant approach, especially since we're already showing the wrong value as @l0rinc says. Just feels like a useless addition to me.
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1742232560)
Previously we disambiguated between length errors and other errors. Since that's not longer true, I think not including the length in the error is the most elegant approach, especially since we're already showing the wrong value as @l0rinc says. Just feels like a useless addition to me.
💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742240161)
Yeah, I tried that before, but getting:
> error: incompatible operand types ('const value_type' (aka 'const uint256') and 'const char[13]')
return os << v ? *v : "std::nullopt";
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742240161)
Yeah, I tried that before, but getting:
> error: incompatible operand types ('const value_type' (aka 'const uint256') and 'const char[13]')
return os << v ? *v : "std::nullopt";
⚠️ maflcko opened an issue: "intermittent issue in wallet_upgradewallet.py AssertionError: [node 2] Node returned unexpected exit code (1) vs (0) when stopping"
(https://github.com/bitcoin/bitcoin/issues/30798)
https://cirrus-ci.com/task/4629002713825280?logs=ci#L3296
```
test 2024-09-03T07:00:27.797000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/ci_container_base/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-g
...
(https://github.com/bitcoin/bitcoin/issues/30798)
https://cirrus-ci.com/task/4629002713825280?logs=ci#L3296
```
test 2024-09-03T07:00:27.797000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/ci_container_base/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-g
...
💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742246864)
Sorry you're right, `return v ? os << *v : os << "std::nullopt";` should work though.
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742246864)
Sorry you're right, `return v ? os << *v : os << "std::nullopt";` should work though.
💬 maflcko commented on pull request "build: Fix / improve coverage scripts":
(https://github.com/bitcoin/bitcoin/pull/30772#issuecomment-2326784390)
CI failure looks unrelated and can be ignored: https://github.com/bitcoin/bitcoin/issues/30798
(https://github.com/bitcoin/bitcoin/pull/30772#issuecomment-2326784390)
CI failure looks unrelated and can be ignored: https://github.com/bitcoin/bitcoin/issues/30798
💬 maflcko commented on pull request "build: Fix / improve coverage scripts":
(https://github.com/bitcoin/bitcoin/pull/30772#issuecomment-2326785530)
review ACK 6e5254cf687578ebb686628cf89f2509d415f04f
(https://github.com/bitcoin/bitcoin/pull/30772#issuecomment-2326785530)
review ACK 6e5254cf687578ebb686628cf89f2509d415f04f
💬 maflcko commented on pull request "build: Fix / improve coverage scripts":
(https://github.com/bitcoin/bitcoin/pull/30772#discussion_r1742252760)
Why not set the parallel tasks, like in the script above?
(https://github.com/bitcoin/bitcoin/pull/30772#discussion_r1742252760)
Why not set the parallel tasks, like in the script above?
💬 ryanofsky commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742256231)
I think I understand why this needs to pass BUILD_TYPE=Debug so the debug flags from the depends system will be applied. But I don't understand why FLAGS_DEBUG variables need to be set to empty stings. How does setting those to empty strings cause MSAN_FLAGS to remain unaltered? What code is altering MSAN_FLAGS if FLAGS_DEBUG variables are unset?
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742256231)
I think I understand why this needs to pass BUILD_TYPE=Debug so the debug flags from the depends system will be applied. But I don't understand why FLAGS_DEBUG variables need to be set to empty stings. How does setting those to empty strings cause MSAN_FLAGS to remain unaltered? What code is altering MSAN_FLAGS if FLAGS_DEBUG variables are unset?
💬 fanquake commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742257436)
So given the Build type change, this line, and the equivalent flag in the other CI config, are redundant and should have been removed. However I don't actually think they should be removed, because it's instead CMake that should be fixed, to remove the assumption that builds using the Debug build type, will always have no optimisations, as this isn't actually the case, for exmaple, in oss-fuzz, or doing a depends build with DEBUG=1. See further discussion in https://github.com/google/oss-fuzz/pu
...
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742257436)
So given the Build type change, this line, and the equivalent flag in the other CI config, are redundant and should have been removed. However I don't actually think they should be removed, because it's instead CMake that should be fixed, to remove the assumption that builds using the Debug build type, will always have no optimisations, as this isn't actually the case, for exmaple, in oss-fuzz, or doing a depends build with DEBUG=1. See further discussion in https://github.com/google/oss-fuzz/pu
...
⚠️ maflcko opened an issue: "Unify -logsourcelocations format"
(https://github.com/bitcoin/bitcoin/issues/30799)
Now that cmake requires out-of-tree builds, it would be nice to unify the `-logsourcelocations` output format.
Currently, with cmake, it includes the `src`, which seems redundant and doesn't match the guix builds.
Out-of-tree builds were always broken in this regard, but now would probably be a good time to unify it to drop the `src` prefix in `logsourcelocations`, so that it matches: (1) legacy-autotools-in-tree behavior (2) legacy- and cmake-guix build behavior.
Ref:
```
$ ./bld-c
...
(https://github.com/bitcoin/bitcoin/issues/30799)
Now that cmake requires out-of-tree builds, it would be nice to unify the `-logsourcelocations` output format.
Currently, with cmake, it includes the `src`, which seems redundant and doesn't match the guix builds.
Out-of-tree builds were always broken in this regard, but now would probably be a good time to unify it to drop the `src` prefix in `logsourcelocations`, so that it matches: (1) legacy-autotools-in-tree behavior (2) legacy- and cmake-guix build behavior.
Ref:
```
$ ./bld-c
...
💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742262723)
Wouldn't
```C++
BOOST_CHECK_EQUAL(from_hex.has_value(), parse_hash_v.has_value());
if (!from_hex.has_value()) continue;
BOOST_CHECK_EQUAL(from_hex.value(), parse_hash_v.value());
```
be simpler after this change as just directly comparing the two?
```C++
BOOST_CHECK_EQUAL(from_hex, parse_hash_v);
```
?
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742262723)
Wouldn't
```C++
BOOST_CHECK_EQUAL(from_hex.has_value(), parse_hash_v.has_value());
if (!from_hex.has_value()) continue;
BOOST_CHECK_EQUAL(from_hex.value(), parse_hash_v.value());
```
be simpler after this change as just directly comparing the two?
```C++
BOOST_CHECK_EQUAL(from_hex, parse_hash_v);
```
?
💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742265812)
was specifically requested to avoid repetition and since they're not mission-critical code: https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1739923996
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742265812)
was specifically requested to avoid repetition and since they're not mission-critical code: https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1739923996
💬 hebasto commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742268255)
Perhaps I used the word 'unaltered' incorrectly. It would be more accurate to say that "flags set in MSAN_FLAGS won't be overridden". The `CMAKE_CXX_FLAGS_DEBUG` follow `CMAKE_CXX_FLAGS` in the compiler invocation string and competing flags may be overridden.
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742268255)
Perhaps I used the word 'unaltered' incorrectly. It would be more accurate to say that "flags set in MSAN_FLAGS won't be overridden". The `CMAKE_CXX_FLAGS_DEBUG` follow `CMAKE_CXX_FLAGS` in the compiler invocation string and competing flags may be overridden.