💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722867989)
> I don't think...
>
> ```c++
> s = ToScript(HexLiteral("0302ff030302ff03"));
> ```
>
> ...will be too bad.
Sure, up to you, it is just a style nit :)
I still think `s = ScriptFromHex("0302ff030302ff03")` is better, because it is shorter, more consistent in the test case, and comes with the same checks at roughly the same CPU cost.
If you really want to change it, maybe it can be done in the follow-up that allows `std::byte` serialization into script? This would also avoid havi
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722867989)
> I don't think...
>
> ```c++
> s = ToScript(HexLiteral("0302ff030302ff03"));
> ```
>
> ...will be too bad.
Sure, up to you, it is just a style nit :)
I still think `s = ScriptFromHex("0302ff030302ff03")` is better, because it is shorter, more consistent in the test case, and comes with the same checks at roughly the same CPU cost.
If you really want to change it, maybe it can be done in the follow-up that allows `std::byte` serialization into script? This would also avoid havi
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722890964)
The commit is modifying the line just above the if, and then the second commit modifies both (and it keeps the second commit a pure rename).
Is the concern that devs running `git blame` will have to exclude multiple commits?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722890964)
The commit is modifying the line just above the if, and then the second commit modifies both (and it keeps the second commit a pure rename).
Is the concern that devs running `git blame` will have to exclude multiple commits?
💬 maflcko commented on pull request "guix: Use DOS newlines for SHA256SUMS files":
(https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-2298267060)
Are you still working on this? There is outstanding review feedback, asking if this is a breaking change in the verification steps? https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-2125882826
(https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-2298267060)
Are you still working on this? There is outstanding review feedback, asking if this is a breaking change in the verification steps? https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-2125882826
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722903409)
Aha, hadn't fully internalized that braces were only allowed to be skip if the then-statement appeared on the same line.
Will fix here and possibly other places if I retouch.
Guess you prefer not hoisting up the `return false;` onto the same line since it disrupts `git blame`?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722903409)
Aha, hadn't fully internalized that braces were only allowed to be skip if the then-statement appeared on the same line.
Will fix here and possibly other places if I retouch.
Guess you prefer not hoisting up the `return false;` onto the same line since it disrupts `git blame`?
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722911225)
> Is the concern that devs running `git blame` will have to exclude multiple commits?
Yes. Repeatedly modifying the same lines of code can not be avoided sometimes, but seems better to minimize, because:
* (as you say) It increases the `git blame` depth, and decreases the usefulness of the `git log -S` parity search (or similar tools)
* Reviewers will have to look at the same line twice, which may take more time
Obviously this conflicts with the desire to keep unrelated changes in sep
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722911225)
> Is the concern that devs running `git blame` will have to exclude multiple commits?
Yes. Repeatedly modifying the same lines of code can not be avoided sometimes, but seems better to minimize, because:
* (as you say) It increases the `git blame` depth, and decreases the usefulness of the `git log -S` parity search (or similar tools)
* Reviewers will have to look at the same line twice, which may take more time
Obviously this conflicts with the desire to keep unrelated changes in sep
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722931351)
My goal was to make it `constexpr`. (`MakeUCharSpan` and `MakeByteSpan` used on the lines below are faux-`constexpr` since they call functions that cannot be `constexpr` as they use `reinterpret_cast`).
A possible but noisier alternative, keeping `string` would be:
```C++
static constexpr std::string out_exp{"04678afdb0"};
```
`string_view` might be a good compromise:
```C++
constexpr std::string_view out_exp{"04678afdb0"};
```
I general having a plain C-array is more terse and narrow
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722931351)
My goal was to make it `constexpr`. (`MakeUCharSpan` and `MakeByteSpan` used on the lines below are faux-`constexpr` since they call functions that cannot be `constexpr` as they use `reinterpret_cast`).
A possible but noisier alternative, keeping `string` would be:
```C++
static constexpr std::string out_exp{"04678afdb0"};
```
`string_view` might be a good compromise:
```C++
constexpr std::string_view out_exp{"04678afdb0"};
```
I general having a plain C-array is more terse and narrow
...
📝 Sjors opened a pull request: "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test"
(https://github.com/bitcoin/bitcoin/pull/30681)
Because #30647 reduced the timewarp attack threshold from 7200s to 600s, our miner code will fail to propose a block template (on testnet4) if the last block of the previous period has a timestamp two hours in the future. This PR fixes that and also adds a test.
In order to add the test, we activate BIP93 on regtest.
In order for the test to run faster, we reduce its difficulty retarget period to 144, the same number that's already used for softfork activation logic. Regtest does not actua
...
(https://github.com/bitcoin/bitcoin/pull/30681)
Because #30647 reduced the timewarp attack threshold from 7200s to 600s, our miner code will fail to propose a block template (on testnet4) if the last block of the previous period has a timestamp two hours in the future. This PR fixes that and also adds a test.
In order to add the test, we activate BIP93 on regtest.
In order for the test to run faster, we reduce its difficulty retarget period to 144, the same number that's already used for softfork activation logic. Regtest does not actua
...
💬 Sjors commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2298316543)
See #30681.
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2298316543)
See #30681.
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722962985)
> I general having a plain C-array is more terse and narrows what the programmer (and compiler) can expect the type to do, so I prefer it in cases like this. But will switch to `string_view` if I retouch.
I guess I disagree that C-arrays should be preferred. Let's recall the beginning of this pull request (https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2208857200) which fixed an issue where a raw C-pointer was passed incorrectly. (C-arrays allow silent conversion into a raw C-poin
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722962985)
> I general having a plain C-array is more terse and narrows what the programmer (and compiler) can expect the type to do, so I prefer it in cases like this. But will switch to `string_view` if I retouch.
I guess I disagree that C-arrays should be preferred. Let's recall the beginning of this pull request (https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2208857200) which fixed an issue where a raw C-pointer was passed incorrectly. (C-arrays allow silent conversion into a raw C-poin
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722964257)
Are you suggesting adding `MakeUCharSpan` so that one could send in a raw array that would otherwise not have `.data()` and `.size()`?
Wouldn't the call to `MakeUCharSpan` have to be re-touched if we switch to using only `std::byte` for `CScript`?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722964257)
Are you suggesting adding `MakeUCharSpan` so that one could send in a raw array that would otherwise not have `.data()` and `.size()`?
Wouldn't the call to `MakeUCharSpan` have to be re-touched if we switch to using only `std::byte` for `CScript`?
💬 Sjors commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1722968233)
Works now.
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1722968233)
Works now.
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722968337)
> Are you suggesting adding `MakeUCharSpan` so that one could send in a raw array that would otherwise not have `.data()` and `.size()`?
No, just to hide the reinterpret cast behind a curtain
> Wouldn't the call to MakeUCharSpan have to be re-touched if we switch to using only std::byte for CScript?
Yes, but that would be just two lines of code touched, as opposed to "many", no?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722968337)
> Are you suggesting adding `MakeUCharSpan` so that one could send in a raw array that would otherwise not have `.data()` and `.size()`?
No, just to hide the reinterpret cast behind a curtain
> Wouldn't the call to MakeUCharSpan have to be re-touched if we switch to using only std::byte for CScript?
Yes, but that would be just two lines of code touched, as opposed to "many", no?
💬 Sjors commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2298367086)
re-ACK af24810eeed397c2fe5f61ef9769e1b7ee0ecdf9
(https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2298367086)
re-ACK af24810eeed397c2fe5f61ef9769e1b7ee0ecdf9
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722984822)
> No, just to hide the reinterpret cast behind a curtain
Aha, so you are concerned about the element type of the container mismatching, but still within allowable realm of `UCharCast`, got it.
Wait.. that will allow me to change `ToScript(HexLiteral<uint8_t>(` into `ToScript(HexLiteral(` - thanks!
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722984822)
> No, just to hide the reinterpret cast behind a curtain
Aha, so you are concerned about the element type of the container mismatching, but still within allowable realm of `UCharCast`, got it.
Wait.. that will allow me to change `ToScript(HexLiteral<uint8_t>(` into `ToScript(HexLiteral(` - thanks!
💬 tdb3 commented on pull request "http: set TCP_NODELAY when creating HTTP server":
(https://github.com/bitcoin/bitcoin/pull/30675#discussion_r1722987831)
Ah, that's right (no hungarian). Thanks sipa
(https://github.com/bitcoin/bitcoin/pull/30675#discussion_r1722987831)
Ah, that's right (no hungarian). Thanks sipa
👍 marcofleon approved a pull request: "fuzz: Faster utxo_snapshot fuzz target"
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2247458898)
Re ACK fa899fb7aa8a14acecadd8936ad5824fa0f697ff
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2247458898)
Re ACK fa899fb7aa8a14acecadd8936ad5824fa0f697ff
📝 Sjors opened a pull request: "Clang 19"
(https://github.com/bitcoin/bitcoin/pull/30682)
This tests https://github.com/bitcoin/bitcoin/pull/30634 and https://github.com/bitcoin/bitcoin/pull/30639 for the TSAN/MSAN failure with `vm.mmap_rnd_bits=32` described in https://github.com/bitcoin/bitcoin/issues/30674.
Since I have two different machines for CI and only one has `vm.mmap_rnd_bits=32`, I normally expect either TSAN or MSAN to fail, but not both. I'll manually restart where needed to get it to run on the right machine.
(https://github.com/bitcoin/bitcoin/pull/30682)
This tests https://github.com/bitcoin/bitcoin/pull/30634 and https://github.com/bitcoin/bitcoin/pull/30639 for the TSAN/MSAN failure with `vm.mmap_rnd_bits=32` described in https://github.com/bitcoin/bitcoin/issues/30674.
Since I have two different machines for CI and only one has `vm.mmap_rnd_bits=32`, I normally expect either TSAN or MSAN to fail, but not both. I'll manually restart where needed to get it to run on the right machine.
✅ Sjors closed a pull request: "Clang 19"
(https://github.com/bitcoin/bitcoin/pull/30682)
(https://github.com/bitcoin/bitcoin/pull/30682)
💬 Sjors commented on pull request "Clang 19":
(https://github.com/bitcoin/bitcoin/pull/30682#issuecomment-2298428620)
(sorry, meant to open this against my own fork)
(https://github.com/bitcoin/bitcoin/pull/30682#issuecomment-2298428620)
(sorry, meant to open this against my own fork)
🤔 BrandonOdiwuor reviewed a pull request: "Improve user dialog when signing multisig psbts"
(https://github.com/bitcoin-core/gui/pull/832#pullrequestreview-2247585717)
Concept ACK
(https://github.com/bitcoin-core/gui/pull/832#pullrequestreview-2247585717)
Concept ACK