💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1682699162)
Ok, so it can be reproduced in Linux with hard links.
However, this leads back to my original question: Why not change the other places as well?
For me, they also fail:
```
$ ln $PWD/../test/functional/p2p_dos_header_tree.py ./test/functional/
$ ./test/functional/p2p_dos_header_tree.py
2024-07-18T11:42:08.340000Z TestFramework (INFO): PRNG seed is: 5276922920789009906
2024-07-18T11:42:08.341000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_9ppp3fwq
...
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1682699162)
Ok, so it can be reproduced in Linux with hard links.
However, this leads back to my original question: Why not change the other places as well?
For me, they also fail:
```
$ ln $PWD/../test/functional/p2p_dos_header_tree.py ./test/functional/
$ ./test/functional/p2p_dos_header_tree.py
2024-07-18T11:42:08.340000Z TestFramework (INFO): PRNG seed is: 5276922920789009906
2024-07-18T11:42:08.341000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_9ppp3fwq
...
👋 paplorinc's pull request is ready for review: "optimization: Precalculate SipHash constant XOR with k0 and k1 in SaltedOutpointHasher"
(https://github.com/bitcoin/bitcoin/pull/30442)
(https://github.com/bitcoin/bitcoin/pull/30442)
🚀 fanquake merged a pull request: "test, refactor: Fix MSVC warning C4101 "unreferenced local variable""
(https://github.com/bitcoin/bitcoin/pull/30464)
(https://github.com/bitcoin/bitcoin/pull/30464)
💬 fanquake commented on pull request "depends: remove Darwin ENV unsetting":
(https://github.com/bitcoin/bitcoin/pull/30451#issuecomment-2236326039)
Assuming that #29723 lands first, the commit here adding a patch for librt will be dropped, as that is also patched out of the CMake side in that PR.
(https://github.com/bitcoin/bitcoin/pull/30451#issuecomment-2236326039)
Assuming that #29723 lands first, the commit here adding a patch for librt will be dropped, as that is also patched out of the CMake side in that PR.
👍 ismaelsadeeq approved a pull request: "refactor: add coinbase constraints to BlockAssembler::Options"
(https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2185605742)
Code review ACK c504b6997b1acc9771ad1f52efaa4be2b4966c6c
The PR description needs to be updated to match the PR.
Left a single question
(https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2185605742)
Code review ACK c504b6997b1acc9771ad1f52efaa4be2b4966c6c
The PR description needs to be updated to match the PR.
Left a single question
💬 ismaelsadeeq commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1682712434)
Is their a reason we are not adding `coinbase_script` into the `BlockCreateOptions` struct?
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1682712434)
Is their a reason we are not adding `coinbase_script` into the `BlockCreateOptions` struct?
💬 craigraw commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2236343826)
> As a user, I'd probably expect to be notified that a transaction isn't in "the connected mempool" and be offered some kind of optional fee-bump (or rebroadcast button, if the transaction hit mempool-expiry).
In general I agree. I was referring specifically to future-dated transactions there, so we may be speaking slightly at cross purposes. But to speak to broadcasted transactions, it's less important to consider the local mempool, since we must assume they are contained in some miners memp
...
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2236343826)
> As a user, I'd probably expect to be notified that a transaction isn't in "the connected mempool" and be offered some kind of optional fee-bump (or rebroadcast button, if the transaction hit mempool-expiry).
In general I agree. I was referring specifically to future-dated transactions there, so we may be speaking slightly at cross purposes. But to speak to broadcasted transactions, it's less important to consider the local mempool, since we must assume they are contained in some miners memp
...
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682745543)
I agree `uint256::FromHex()` is more self-explanatory than `uint256S()`. Making it return `std::optional` might be good if we want stricter parsing and non-fatal failures. This PR feels large enough for now though.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682745543)
I agree `uint256::FromHex()` is more self-explanatory than `uint256S()`. Making it return `std::optional` might be good if we want stricter parsing and non-fatal failures. This PR feels large enough for now though.
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682755930)
Yes, all of this should happen later, because it is unrelated to fixing possible UB/crashes (the goal of this pull).
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682755930)
Yes, all of this should happen later, because it is unrelated to fixing possible UB/crashes (the goal of this pull).
💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682767232)
Fixed as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682767232)
Fixed as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.
💬 paplorinc commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682771932)
Since a random access vector seems to resolve the problem, is there any reason we cannot change the `outpoints` definition to a vector instead?
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682771932)
Since a random access vector seems to resolve the problem, is there any reason we cannot change the `outpoints` definition to a vector instead?
💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682772285)
As of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d I have stopped overloading `uint256S()`:
Changed from `consteval uint256 uint256S(const char *str)` -> `consteval explicit uint256(const char (&str)[65])` and `base_blob(std::string_view str)`
This means the string width is enforced at compile time. (The new parsing code is much more strict and also asserts on the length).
Applied uint256S -> uint256 conversion where applicable, removing "0x"-prefixes.
TODO: if there is agreement on the curr
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682772285)
As of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d I have stopped overloading `uint256S()`:
Changed from `consteval uint256 uint256S(const char *str)` -> `consteval explicit uint256(const char (&str)[65])` and `base_blob(std::string_view str)`
This means the string width is enforced at compile time. (The new parsing code is much more strict and also asserts on the length).
Applied uint256S -> uint256 conversion where applicable, removing "0x"-prefixes.
TODO: if there is agreement on the curr
...
💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682773691)
More strict as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682773691)
More strict as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.
💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682775910)
`consteval` parsing now done inside `base_blob(std::string_view)` as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682775910)
`consteval` parsing now done inside `base_blob(std::string_view)` as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.
💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682777414)
Fixed as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682777414)
Fixed as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d.
💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682782077)
Still kept it as an overload as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d. If you prefer different names for runtime/compile time, which do you suggest?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682782077)
Still kept it as an overload as of 10baae11ecc376f2250d4a51d2bbfeea56c0a31d. If you prefer different names for runtime/compile time, which do you suggest?
💬 hebasto commented on pull request "depends: Amend handling flags environment variables":
(https://github.com/bitcoin/bitcoin/pull/30477#issuecomment-2236426928)
> It would be good to show an example of what this actually fixes.
It is required only for the main CMake-based build system.
For instance, CMake [assumes](https://cmake.org/cmake/help/latest/envvar/CXXFLAGS.html) that CXXFLAGS affects the content of the CMAKE_CXX_FLAGS variable. However, without this change, the CMAKE_CXX_FLAGS_RELEASE or CMAKE_CXX_FLAGS_DEBUG will be affected instead.
The PR description has been updated.
(https://github.com/bitcoin/bitcoin/pull/30477#issuecomment-2236426928)
> It would be good to show an example of what this actually fixes.
It is required only for the main CMake-based build system.
For instance, CMake [assumes](https://cmake.org/cmake/help/latest/envvar/CXXFLAGS.html) that CXXFLAGS affects the content of the CMAKE_CXX_FLAGS variable. However, without this change, the CMAKE_CXX_FLAGS_RELEASE or CMAKE_CXX_FLAGS_DEBUG will be affected instead.
The PR description has been updated.
💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2236512167)
@ismaelsadeeq thanks, updated the description.
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2236512167)
@ismaelsadeeq thanks, updated the description.
💬 hodlinator commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2236517862)
(Fixed QT/GUI CI errors).
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2236517862)
(Fixed QT/GUI CI errors).
💬 maflcko commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682825302)
Why add a consteval overload here? Generally I am not a fan of adding test-only code (code that is only used in tests) to the real program. Performance or exe-size shouldn't matter for tests, unless it is significant. If you still want to add it to the tests, that is fine, but then please move it to the test code. But please change the function name of the test-only function to something else. It seems odd that test-only code forces real code to be renamed.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682825302)
Why add a consteval overload here? Generally I am not a fan of adding test-only code (code that is only used in tests) to the real program. Performance or exe-size shouldn't matter for tests, unless it is significant. If you still want to add it to the tests, that is fine, but then please move it to the test code. But please change the function name of the test-only function to something else. It seems odd that test-only code forces real code to be renamed.