💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717564420)
Now part of b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717564420)
Now part of b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717568070)
Done.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717568070)
Done.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717420847)
Changing it to a requires-clause as per https://github.com/bitcoin/bitcoin/pull/30377/files#r1717324499 with additional check for null terminator at the end. Don't think someone would do:
```C++
constexpr char my_hex[] = {'f', 'f'};
constexpr std::array<uint8_t, 1> arr = ArrayFromHex(my_hex);
```
when they could just do:
```C++
constexpr std::array<uint8_t, 1> arr = {0xff};
```
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717420847)
Changing it to a requires-clause as per https://github.com/bitcoin/bitcoin/pull/30377/files#r1717324499 with additional check for null terminator at the end. Don't think someone would do:
```C++
constexpr char my_hex[] = {'f', 'f'};
constexpr std::array<uint8_t, 1> arr = ArrayFromHex(my_hex);
```
when they could just do:
```C++
constexpr std::array<uint8_t, 1> arr = {0xff};
```
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717562300)
Now explained in commit message 0a82c18457ec81e911b835b2ac76ad7475384983 as suggested in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717386520.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717562300)
Now explained in commit message 0a82c18457ec81e911b835b2ac76ad7475384983 as suggested in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717386520.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717458216)
Clang unfortunately gives me: ``candidate function template not viable: no known conversion from 'const char[65]' to 'ConstevalHexLiteral' for 1st argument``
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717458216)
Clang unfortunately gives me: ``candidate function template not viable: no known conversion from 'const char[65]' to 'ConstevalHexLiteral' for 1st argument``
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717462054)
I think `*FromHex` jives well with `uint256::FromHex()`.. only thing is the latter stores the bytes in reverse order. :face_in_clouds:
Could go for `VectorFromHex`, but there is already some weak precedence in the type `VecDeque`.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717462054)
I think `*FromHex` jives well with `uint256::FromHex()`.. only thing is the latter stores the bytes in reverse order. :face_in_clouds:
Could go for `VectorFromHex`, but there is already some weak precedence in the type `VecDeque`.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717565594)
I'm burned by old MSVC warnings (probably not current) "implicit conversion to bool" so I'd rather not.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717565594)
I'm burned by old MSVC warnings (probably not current) "implicit conversion to bool" so I'd rather not.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717578019)
Done in latest.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717578019)
Done in latest.
🤔 furszy reviewed a pull request: "[WIP] p2p: send not_found msgs for unknown, pruned or unwilling to share blocks"
(https://github.com/bitcoin/bitcoin/pull/30385#pullrequestreview-2239237401)
Thanks for the comment gmaxwell.
> I think if you find yourself thinking sending a not found is a solution to a problem, you've got a mistake somewhere else.
Yeah, thats actually how this working path started #28120.
> I believed the current behavior was this: If a node is non-pruned it will have all blocks, no issue. If a node is pruned it will respond to requests for the last 288 which are protocol required and will disconnect any peer that asks for a block outside of the window. Agai
...
(https://github.com/bitcoin/bitcoin/pull/30385#pullrequestreview-2239237401)
Thanks for the comment gmaxwell.
> I think if you find yourself thinking sending a not found is a solution to a problem, you've got a mistake somewhere else.
Yeah, thats actually how this working path started #28120.
> I believed the current behavior was this: If a node is non-pruned it will have all blocks, no issue. If a node is pruned it will respond to requests for the last 288 which are protocol required and will disconnect any peer that asks for a block outside of the window. Agai
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717580883)
Further changed to just:
```C++
constexpr explicit XOnlyPubKey(Span<const unsigned char> bytes) : m_keydata{bytes} {}
```
Since `base_blob(Span<const unsigned char>)` does the exact length assert internally.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717580883)
Further changed to just:
```C++
constexpr explicit XOnlyPubKey(Span<const unsigned char> bytes) : m_keydata{bytes} {}
```
Since `base_blob(Span<const unsigned char>)` does the exact length assert internally.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717589830)
> So I guess this commit drops support for XCode on macOS Ventura 13. Not sure if doc/build-osx.md needs to be adjusted, similar to https://github.com/bitcoin/bitcoin/pull/29934/files
According to my earlier (pending and therefore out of order) message, https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716900164, the clang version is not the issue. So simply changing to `llvm@18` in the docs might not be correct.
What is your clang version after the XCode upgrade @stickies-v?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717589830)
> So I guess this commit drops support for XCode on macOS Ventura 13. Not sure if doc/build-osx.md needs to be adjusted, similar to https://github.com/bitcoin/bitcoin/pull/29934/files
According to my earlier (pending and therefore out of order) message, https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716900164, the clang version is not the issue. So simply changing to `llvm@18` in the docs might not be correct.
What is your clang version after the XCode upgrade @stickies-v?
👍 tdb3 approved a pull request: "doc: mention bip94 support"
(https://github.com/bitcoin/bitcoin/pull/30655#pullrequestreview-2239325669)
ACK 99eeb51bf65fa00ee863f32d70f478bb9db0e351
(https://github.com/bitcoin/bitcoin/pull/30655#pullrequestreview-2239325669)
ACK 99eeb51bf65fa00ee863f32d70f478bb9db0e351
💬 brunoerg commented on pull request "coins: Simplify std::move to ternary in `coins.cpp`":
(https://github.com/bitcoin/bitcoin/pull/30656#discussion_r1717638941)
@TheCharlatan +1
Also, the current code is clearer for me, especially due to the comment.
(https://github.com/bitcoin/bitcoin/pull/30656#discussion_r1717638941)
@TheCharlatan +1
Also, the current code is clearer for me, especially due to the comment.
💬 ajtowns commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2290151775)
> You also mentioned wasting inbound slots, but requiring `INV+GETDATA` will not solve that - the mass connect tool will be updated to do `INV+GETDATA+TX` instead of just `TX`. Those are short-lived connections anyway.
Sending INV and waiting for a GETDATA does mitigate this substantially: when the txs are duplicates, you're now only receiving the w/txid instead of the full transaction, reducing the wasted bandwidth by perhaps a factor of at least ~4x (32B vs ~128B for parent txid/sig/output)
...
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2290151775)
> You also mentioned wasting inbound slots, but requiring `INV+GETDATA` will not solve that - the mass connect tool will be updated to do `INV+GETDATA+TX` instead of just `TX`. Those are short-lived connections anyway.
Sending INV and waiting for a GETDATA does mitigate this substantially: when the txs are duplicates, you're now only receiving the w/txid instead of the full transaction, reducing the wasted bandwidth by perhaps a factor of at least ~4x (32B vs ~128B for parent txid/sig/output)
...
💬 ajtowns commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#discussion_r1717682181)
`ReceivedResponse` already does this, could just have it return true in that case, and false otherwise (instead of void), and issue the error if neither the txid or wtxid resulted in a true result?
(https://github.com/bitcoin/bitcoin/pull/30572#discussion_r1717682181)
`ReceivedResponse` already does this, could just have it return true in that case, and false otherwise (instead of void), and issue the error if neither the txid or wtxid resulted in a true result?
💬 agroce commented on issue "fuzz: `script`: Assertion `!extract_destination_ret' failed.":
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2290178998)
@maflcko @dergoegge this is actually a question worth trying muttfuzz on: I think clearly full suite / max len fuzzing kills more mutants. but I can see how big the cost is for max len 50 runs, and how common mutants that are much easier to detect with short inputs only are. that might be worth seeing, since the gap in detection time here does seem to be huge (I have many finds with 50 length corpus/max_len, some in less than 10 minutes, and the original run with full corpus/no limit is still
...
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2290178998)
@maflcko @dergoegge this is actually a question worth trying muttfuzz on: I think clearly full suite / max len fuzzing kills more mutants. but I can see how big the cost is for max len 50 runs, and how common mutants that are much easier to detect with short inputs only are. that might be worth seeing, since the gap in detection time here does seem to be huge (I have many finds with 50 length corpus/max_len, some in less than 10 minutes, and the original run with full corpus/no limit is still
...
🤔 tdb3 reviewed a pull request: "test: add functional test for XORed block/undo files (`-blocksxor` option)"
(https://github.com/bitcoin/bitcoin/pull/30657#pullrequestreview-2239353746)
Approach ACK.
Nice additions.
Left a comment and a nit.
(https://github.com/bitcoin/bitcoin/pull/30657#pullrequestreview-2239353746)
Approach ACK.
Nice additions.
Left a comment and a nit.
💬 tdb3 commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1717724161)
The help for `-blocksxor` mentions `The created XOR-key will be zeros for an existing blocksdir or when `-blocksxor=0` is set`.
After the `verifychain`, could test that a new `xor.dat` is created (consisting of all zeros) when the node is started with no `xor.dat` and with `-blocksxor=0`. Something like:
```python
assert read_xor_key(node=node) == bytes(8)
```
or maybe lift `NUM_XOR_BYTES` out of `read_xor_key` to enable
```python
assert read_xor_key(node=node) == bytes(NUM_XOR_BYTES)
...
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1717724161)
The help for `-blocksxor` mentions `The created XOR-key will be zeros for an existing blocksdir or when `-blocksxor=0` is set`.
After the `verifychain`, could test that a new `xor.dat` is created (consisting of all zeros) when the node is started with no `xor.dat` and with `-blocksxor=0`. Something like:
```python
assert read_xor_key(node=node) == bytes(8)
```
or maybe lift `NUM_XOR_BYTES` out of `read_xor_key` to enable
```python
assert read_xor_key(node=node) == bytes(NUM_XOR_BYTES)
...
💬 tdb3 commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1717654069)
nitty nit: Since this is being moved, could update to use a more descriptive function name (e.g. `xor_bytes`). Feel free to ignore.
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1717654069)
nitty nit: Since this is being moved, could update to use a more descriptive function name (e.g. `xor_bytes`). Feel free to ignore.
💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2290369678)
> I'm a little surprised that the effect is quite so small
@gmaxwell I was as well, so I ran the same benchmark with `-reindex` as well and it gave me similar results:
```
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|:---|---:|---:|---:|---:|
| `echo 81508954f1f89f95b6fdca10c3c471cdb6566c80 && \
./src/bitcoind -datadir=/home/user/.bitcoin \
-reindex \
-dbcache=16384 \
-printtoconsole=0 \
-connect=0 \
-stopatheight=820000` | 40765.075 ± 132.452 | 40671.417 | 40858.733 |
...
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2290369678)
> I'm a little surprised that the effect is quite so small
@gmaxwell I was as well, so I ran the same benchmark with `-reindex` as well and it gave me similar results:
```
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|:---|---:|---:|---:|---:|
| `echo 81508954f1f89f95b6fdca10c3c471cdb6566c80 && \
./src/bitcoind -datadir=/home/user/.bitcoin \
-reindex \
-dbcache=16384 \
-printtoconsole=0 \
-connect=0 \
-stopatheight=820000` | 40765.075 ± 132.452 | 40671.417 | 40858.733 |
...