💬 TheCharlatan commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1717554488)
Please stick to the symbol naming conventions in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c - specifically use snake_case everywhere.
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1717554488)
Please stick to the symbol naming conventions in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c - specifically use snake_case everywhere.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717569863)
Done, slightly, `vchCiphertext` -> `chCiphertext` (already some precedence for `ch`-prefix). Also changed `vchSalt` -> `salt` in some places.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717569863)
Done, slightly, `vchCiphertext` -> `chCiphertext` (already some precedence for `ch`-prefix). Also changed `vchSalt` -> `salt` in some places.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717575383)
Added 4 of you in b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795, hopefully you don't mind the company. :)
@maflcko let me know if you prefer a different identifier to make GitHub rendering prettier.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717575383)
Added 4 of you in b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795, hopefully you don't mind the company. :)
@maflcko let me know if you prefer a different identifier to make GitHub rendering prettier.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717568246)
Done.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717568246)
Done.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717562892)
Now explained in commit message 0a82c18457ec81e911b835b2ac76ad7475384983
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717562892)
Now explained in commit message 0a82c18457ec81e911b835b2ac76ad7475384983
💬 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?