💬 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.
💬 maflcko commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682842021)
Happy to push a change that changes this, if someone writes one. Also, I am happy to review a pull request doing the change, if someone opens one.
But this requires changing more lines of code, so I didn't find it worthwhile to do by myself.
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682842021)
Happy to push a change that changes this, if someone writes one. Also, I am happy to review a pull request doing the change, if someone opens one.
But this requires changing more lines of code, so I didn't find it worthwhile to do by myself.
💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1682842951)
No strong reason, but the coinbase script is different for every block, unlike the other stuff we pass to `BlockCreateOptions`. It seems worthy of a positional argument.
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1682842951)
No strong reason, but the coinbase script is different for every block, unlike the other stuff we pass to `BlockCreateOptions`. It seems worthy of a positional argument.
💬 hodlinator commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682843219)
Good call, will fix.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682843219)
Good call, will fix.
💬 fanquake commented on pull request "guix: bump time-machine to e6e70abe65850da0ae69428654375d367088ef5e":
(https://github.com/bitcoin/bitcoin/pull/30452#issuecomment-2236543931)
Bumped this further as https://git.savannah.gnu.org/cgit/guix.git/commit/?id=49fa48eed78c50d9674e9868e5a2b3e029a6e403 landed.
(https://github.com/bitcoin/bitcoin/pull/30452#issuecomment-2236543931)
Bumped this further as https://git.savannah.gnu.org/cgit/guix.git/commit/?id=49fa48eed78c50d9674e9868e5a2b3e029a6e403 landed.
💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2236563617)
Guix Build (aarch64):
```bash
e9a70ec7c66652aa96facc2fe3117fb177cb61e6d28fadc48893546f9cebf6cb guix-build-218e69c705d7/output/aarch64-linux-gnu/SHA256SUMS.part
0fea765c772ee00ec37d4efde6296e173a8a765792534a731e64b253effd9242 guix-build-218e69c705d7/output/aarch64-linux-gnu/bitcoin-218e69c705d7-aarch64-linux-gnu-debug.tar.gz
1689943e2b3caa664a9bb5c137ffc161252b253568b563d5a4433e2ac55174ec guix-build-218e69c705d7/output/aarch64-linux-gnu/bitcoin-218e69c705d7-aarch64-linux-gnu.tar.gz
6934a4
...
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2236563617)
Guix Build (aarch64):
```bash
e9a70ec7c66652aa96facc2fe3117fb177cb61e6d28fadc48893546f9cebf6cb guix-build-218e69c705d7/output/aarch64-linux-gnu/SHA256SUMS.part
0fea765c772ee00ec37d4efde6296e173a8a765792534a731e64b253effd9242 guix-build-218e69c705d7/output/aarch64-linux-gnu/bitcoin-218e69c705d7-aarch64-linux-gnu-debug.tar.gz
1689943e2b3caa664a9bb5c137ffc161252b253568b563d5a4433e2ac55174ec guix-build-218e69c705d7/output/aarch64-linux-gnu/bitcoin-218e69c705d7-aarch64-linux-gnu.tar.gz
6934a4
...
💬 maflcko commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682878016)
A more flexible alternative would be to just accept a string_view and rely on the length assert. However, I guess this prints a more confusing compile error.
Seems fine to change later to string_view, if this is needed.
However, I wonder if you can replace `65` by `WIDTH+1`?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682878016)
A more flexible alternative would be to just accept a string_view and rely on the length assert. However, I guess this prints a more confusing compile error.
Seems fine to change later to string_view, if this is needed.
However, I wonder if you can replace `65` by `WIDTH+1`?