💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729491937)
@stickies-v:
> It would, in my view, make it more confusing though, by hiding that these operators are in fact consteval. I might be missing some nuance here, though?

Sorry for not addressing this before, was a bit rushed in my previous post.
My understanding is the `Hex`-instantiations are `consteval`, and the operators are *instantiated* at compile time, but the last two are commonly *executed* at runtime (with a compile time baked `Hex`-value).

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729499254)
Will see if all compilers are fine with removing `util::detail::Hex(...)` here. Hopefully they are!

I prefer documenting the full type in for these specific tests, but admit it is quite verbose.

✅ Christewart closed a pull request: "Implement 64 bit arithmetic op codes in the Script interpreter"
(https://github.com/bitcoin/bitcoin/pull/29221)

💬 Christewart commented on pull request "Implement 64 bit arithmetic op codes in the Script interpreter":
(https://github.com/bitcoin/bitcoin/pull/29221#issuecomment-2307855599)
Too speculative for now https://delvingbitcoin.org/t/64-bit-arithmetic-soft-fork/397/53

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1729563315)
Hm, doesn't this code catch this already?

```
FILE* file{fsbridge::fopen(temppath, "wb")};
AutoFile afile{file};
if (afile.IsNull()) {
```

I get this here:

```
$ src/bitcoin-cli dumptxoutset \~/Downloads/utxo.dat latest
error code: -8
error message:
Couldn't open file /Users/XXX/Library/Application Support/Bitcoin/~/Downloads/utxo.dat.incomplete for writing.
```

Can you give a more detailed example to reproduce this maybe?

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1729563346)
done

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1729563423)
Leaving this for a follow-up.

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1729563485)
I agree that `-height=XXXX` is better but having `-rollback` and `-height=XXXXX` seems worse than `rollback` and `-rollback=XXXXX` and of course just `-height` doesn't make sense. So leaving this as is for now.

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1729563501)
done

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1729563563)
added

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1729563596)
fixed

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#issuecomment-2307876124)
I addressed @Sjors 's feedback and rebased so the behavior can be tested with the mainnet params.

> This can wait for a followup, but can you add -rpcclienttimeout=0 to all the examples?

Done

> But that's because this PR wasn't rebased after https://github.com/bitcoin/bitcoin/pull/28553 got merged.

Rebased for easier testing.

👍 pablomartin4btc approved a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2258177952)
tACK 41051290ab3b6c36312cec26a27f787cea9961b4

Cross built for Win11 on WSL 24.04, performed `install`, `deploy`, run `ctest` and `test_bitcoin` on both WSL and WIndows.

Small details regarding the docs:
- In "Building for 64-bit Windows" doc, building depends got changed to `gmake`, but not in depends doc.
- Also there's no reference to `ctest` in `build-unix.md` except at the end as part of the "Setup and Build Example: Arch Linux" , should not be at the beginning in the "To Build" sect
...

✅ achow101 closed a pull request: "contrib: add dockerfile for building image fron source code"
(https://github.com/bitcoin/bitcoin/pull/30702)

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729584258)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1728787408

This is an interesting idea. I kind of think now that we have support for these suffixes, we should just add an `_rhex` reverse hex suffix and drop this constructor entirely, avoid the footgun of having two constructors where one reverses its input and the other does not.

Also, IMO if adopting this suggestion it would be a little better to keep detail namespace than to expose Hex class as something that would be reaso
...

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729536195)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729064946

> I want to explicitly enforce compile time where possible, hence the consteval.

I don't think this is a good idea. IMO, the interface should simple and consistent, not noisy with unnecessary / unexplained differences that obscure more real and relevant differences between the functions. It's not good to expose internal implementation details unnecessarily or to tell compilers how to do their jobs unnecessarily. The c
...

👍 tdb3 approved a pull request: "test: Add time-timewarp-attack boundary cases"
(https://github.com/bitcoin/bitcoin/pull/30698#pullrequestreview-2258270996)
ACK 31378d44f44cabc576bf92ddd61afde4b528de77

Great addition (testing around `MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP`).
Tested locally (and sanity checked that the assert would catch removing the `-1` from `bad_block.nTime = t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP - 1`).

Left one comment, but it doesn't block the ACK.

💬 tdb3 commented on pull request "test: Add time-timewarp-attack boundary cases":
(https://github.com/bitcoin/bitcoin/pull/30698#discussion_r1729632270)
I'm not immediately seeing why the previous check (for the `bad_block.nTime = t` case) is needed anymore now that the check below is being added (`bad_block.nTime = t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP - 1` case). Maybe I'm overlooking something?

If not, then could add a commit that removes the check for the `bad_block.nTime = t` case.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729787316)
Not sure about how `_rhex` would work, if it returned `std::array<std::byte, N>` one could accidentally substitute for `_hex`. Keeping `Hex` in `detail` for now though.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729787403)
Done!