💬 MarnixCroes commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1717223832)
nit: maybe a more descriptive text would be good?
something like `No wallets that need to be migrated`
for people who don't know about the migration and why/what it is.
seeing _No wallets available_ while they have wallet(s) might seem wrong/confusing.
sry for commenting post-merge
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1717223832)
nit: maybe a more descriptive text would be good?
something like `No wallets that need to be migrated`
for people who don't know about the migration and why/what it is.
seeing _No wallets available_ while they have wallet(s) might seem wrong/confusing.
sry for commenting post-merge
🤔 pablomartin4btc reviewed a pull request: "validation: assumeutxo params mainnet"
(https://github.com/bitcoin/bitcoin/pull/28553#pullrequestreview-2238694990)
tACK 1610643c8b37a9f674b236cfa79abf8f8aaf1410
Checksum matches downloaded file and performed `loadtxoutset` on a new node.
I'm still performing some other tests.
(https://github.com/bitcoin/bitcoin/pull/28553#pullrequestreview-2238694990)
tACK 1610643c8b37a9f674b236cfa79abf8f8aaf1410
Checksum matches downloaded file and performed `loadtxoutset` on a new node.
I'm still performing some other tests.
💬 ryanofsky commented on pull request "test: [refactor] Use g_rng/m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2289335140)
Code review ACK facc85b766dec2481384c878ccbf1696a8386e10. But I think we can eliminate the global now, and I think would be good to do it in this PR to changing affected tests more than once. I posted a branch https://github.com/ryanofsky/bitcoin/commits/pr/rng which I hope you can steal. It has the following commits:
- a334a9b9bd0d8ee520f9e2769534625a5d147705 test: Add m_rng alias for the global random context
- c38390fb43e40913afa2072c7986ea80da1d4d38 test: refactor: Make unsigned promotio
...
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2289335140)
Code review ACK facc85b766dec2481384c878ccbf1696a8386e10. But I think we can eliminate the global now, and I think would be good to do it in this PR to changing affected tests more than once. I posted a branch https://github.com/ryanofsky/bitcoin/commits/pr/rng which I hope you can steal. It has the following commits:
- a334a9b9bd0d8ee520f9e2769534625a5d147705 test: Add m_rng alias for the global random context
- c38390fb43e40913afa2072c7986ea80da1d4d38 test: refactor: Make unsigned promotio
...
💬 achow101 commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#issuecomment-2289394756)
Updated for testnet4.
Also refreshed the seeds for pre-28.0.
(https://github.com/bitcoin/bitcoin/pull/30008#issuecomment-2289394756)
Updated for testnet4.
Also refreshed the seeds for pre-28.0.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717020191)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716616576:
Enough casing changes for one PR. :)
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717020191)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716616576:
Enough casing changes for one PR. :)
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716886175)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716693711:
@maflcko: Might introduce `VecFromHex` for tests but I like that this constant is fully compile time.
@paplorinc: I have been trying out exactly that approach quite a bit, but was [warned by maflcko](https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1692989490) that there may be lying dragons among the different serializations of `vector`/`array`/`span`/`Span`. Also had some annoying MSVC compiler errors w
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716886175)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716693711:
@maflcko: Might introduce `VecFromHex` for tests but I like that this constant is fully compile time.
@paplorinc: I have been trying out exactly that approach quite a bit, but was [warned by maflcko](https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1692989490) that there may be lying dragons among the different serializations of `vector`/`array`/`span`/`Span`. Also had some annoying MSVC compiler errors w
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716895696)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716610850:
I think it's more rigorous/strict to not use `ParseHex` on both left & right sides of the comparison.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716895696)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716610850:
I think it's more rigorous/strict to not use `ParseHex` on both left & right sides of the comparison.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716890935)
Clang and GCC don't, but MSVC does. :(
Removed in this function in latest push.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716890935)
Clang and GCC don't, but MSVC does. :(
Removed in this function in latest push.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716887365)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716685019:
I'll meet you half-way with `constexpr auto`. :)
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716887365)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716685019:
I'll meet you half-way with `constexpr auto`. :)
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716903532)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716703571:
Tried to start switching to `std::byte` after your comment, but it quickly propagated to untouched files.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716903532)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716703571:
Tried to start switching to `std::byte` after your comment, but it quickly propagated to untouched files.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717015082)
Attempted to minimize the diff in latest push, but went with `VecFromHex` for string literals and introduced a new `ScriptFromHexStr` for the 2 runtime validated call sites.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717015082)
Attempted to minimize the diff in latest push, but went with `VecFromHex` for string literals and introduced a new `ScriptFromHexStr` for the 2 runtime validated call sites.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716875704)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716678159:
Thanks, better get this part right. :)
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716875704)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716678159:
Thanks, better get this part right. :)
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716900164)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716752262:
Weird, actually works for me on same clang version as @stickies-v, but under Linux:
```
$ clang --version
clang version 17.0.6
Target: x86_64-unknown-linux-gnu
```
Maybe it's the standard library that is somehow different (missing some `constexpr`)?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716900164)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716752262:
Weird, actually works for me on same clang version as @stickies-v, but under Linux:
```
$ clang --version
clang version 17.0.6
Target: x86_64-unknown-linux-gnu
```
Maybe it's the standard library that is somehow different (missing some `constexpr`)?
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717012368)
> `ScriptFromHex` failed for empty inputs.
How come? `ParseHex("")` returns `0` as verified by **util_tests.cpp**. Besides that, I think it's not really what we are testing in this file.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717012368)
> `ScriptFromHex` failed for empty inputs.
How come? `ParseHex("")` returns `0` as verified by **util_tests.cpp**. Besides that, I think it's not really what we are testing in this file.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717314517)
Taken in latest push.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717314517)
Taken in latest push.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717316492)
Taken in latest push.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717316492)
Taken in latest push.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717317988)
Taken in latest push.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717317988)
Taken in latest push.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717318999)
Taken in latest push, should add co-authorship on next re-touch.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717318999)
Taken in latest push, should add co-authorship on next re-touch.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717320370)
Taken in latest push.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717320370)
Taken in latest push.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717327999)
Updated with slightly different string in latest push and removed comment about consistency above as well.
```C++
throw "Only lowercase hex digits are allowed, for consistency";
```
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717327999)
Updated with slightly different string in latest push and removed comment about consistency above as well.
```C++
throw "Only lowercase hex digits are allowed, for consistency";
```