💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1721074014)
Done in #30673 30673
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1721074014)
Done in #30673 30673
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1721074195)
Revived in #30673
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1721074195)
Revived in #30673
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1721074348)
Done in #30673
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1721074348)
Done in #30673
💬 gmaxwell commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2295433373)
@vasild I don't think ariaid's initial justification for the PR was the most important one. Sometimes people propose doing the right things for less than the most important reasons. If one were to judge things only by their initial proposals then it would open a very easy attack on development-- just keep proposing good things with bad justification to make sure they won't get done. :)
The amount of CPU-work per byte sent might only be increased by 50% by forcing INV use. E.g. say an at
...
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2295433373)
@vasild I don't think ariaid's initial justification for the PR was the most important one. Sometimes people propose doing the right things for less than the most important reasons. If one were to judge things only by their initial proposals then it would open a very easy attack on development-- just keep proposing good things with bad justification to make sure they won't get done. :)
The amount of CPU-work per byte sent might only be increased by 50% by forcing INV use. E.g. say an at
...
💬 gmaxwell commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2295439986)
Private broadcast can also reduce privacy. I think the weakness is a tossup vs the obvious alternative (run ordinary connections exclusively over Tor) so it's not a reason to not implement the feature, but it's worth discussing in order to understand the limits and see if mitigations are possible.
The issue on my mind is that Tor has little to no resistance to traffic analysis. The small amount of resistance it has is by quantizing messages into fixed size cells. Otherwise, it essentially
...
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2295439986)
Private broadcast can also reduce privacy. I think the weakness is a tossup vs the obvious alternative (run ordinary connections exclusively over Tor) so it's not a reason to not implement the feature, but it's worth discussing in order to understand the limits and see if mitigations are possible.
The issue on my mind is that Tor has little to no resistance to traffic analysis. The small amount of resistance it has is by quantizing messages into fixed size cells. Otherwise, it essentially
...
✅ willcl-ark closed an issue: "doc: deduplicate list of chain/network strings in RPC/parameter help texts"
(https://github.com/bitcoin/bitcoin/issues/30645)
(https://github.com/bitcoin/bitcoin/issues/30645)
💬 willcl-ark commented on issue "doc: deduplicate list of chain/network strings in RPC/parameter help texts":
(https://github.com/bitcoin/bitcoin/issues/30645#issuecomment-2295837411)
Closing since https://github.com/bitcoin/bitcoin/pull/30648 was merged
(https://github.com/bitcoin/bitcoin/issues/30645#issuecomment-2295837411)
Closing since https://github.com/bitcoin/bitcoin/pull/30648 was merged
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721317181)
Taken (mostly).
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721317181)
Taken (mostly).
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721317268)
Done
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721317268)
Done
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721324596)
Indeed that's nicer.
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721324596)
Indeed that's nicer.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721360325)
I guess we could add an interim `HexLiteralU`, but that would decrease the motivation to adapt to `std::byte`. :)
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721360325)
I guess we could add an interim `HexLiteralU`, but that would decrease the motivation to adapt to `std::byte`. :)
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721355157)
Yeah, when it came to short/empty hex strings I've been tempted to use something like `std::vector{0xFF}` but felt inconsistent. `valtype` is a good find, but doesn't provide so much of a win once we have full `std::array<std::byte>` support in `CSscript`:
``valtype{0xFF}`` vs
``HexLiteral("FF")``
``valtype{}`` vs
``HexLiteral("")``
(I think switching to base 10 is or using `valtype(36, 0xff);` is too inconsistent). So keeping as-is for now.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721355157)
Yeah, when it came to short/empty hex strings I've been tempted to use something like `std::vector{0xFF}` but felt inconsistent. `valtype` is a good find, but doesn't provide so much of a win once we have full `std::array<std::byte>` support in `CSscript`:
``valtype{0xFF}`` vs
``HexLiteral("FF")``
``valtype{}`` vs
``HexLiteral("")``
(I think switching to base 10 is or using `valtype(36, 0xff);` is too inconsistent). So keeping as-is for now.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721337019)
I would be happy if you took the ball on that. But would appreciate if you worked on it in a personal branch (still pushing to GitHub for CI) and waited a few days more with making it into a PR.
~6 pushes ago on this PR I had commit 5be34598c4683b2b44f607b28592a9e68e089761 which added targeted `std::array` support to **script.h**. It is conservative in that it doesn't publicly expose the possibility of appending raw `std::span`. IIRC MSVC had some issue with it, but could have been a differen
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721337019)
I would be happy if you took the ball on that. But would appreciate if you worked on it in a personal branch (still pushing to GitHub for CI) and waited a few days more with making it into a PR.
~6 pushes ago on this PR I had commit 5be34598c4683b2b44f607b28592a9e68e089761 which added targeted `std::array` support to **script.h**. It is conservative in that it doesn't publicly expose the possibility of appending raw `std::span`. IIRC MSVC had some issue with it, but could have been a differen
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721367896)
These lines are part of the scripted diff commit (67fc994bedf14e360b3e51fa1a71dc6c1684b532), and I'd rather not complicate the regexps just for that.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721367896)
These lines are part of the scripted diff commit (67fc994bedf14e360b3e51fa1a71dc6c1684b532), and I'd rather not complicate the regexps just for that.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721364492)
Will do if I retouch.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721364492)
Will do if I retouch.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720779733)
As I said here: https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2292367131
> I tried experimenting with user defined literals in response now but ran into issues with both making them `consteval` and accepting a `size_t`-templated char-array argument.
If we are to return a `std::array` I think we need to take the size as a template argument I think. And no compiler seems to accept `consteval` user defined literals.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720779733)
As I said here: https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2292367131
> I tried experimenting with user defined literals in response now but ran into issues with both making them `consteval` and accepting a `size_t`-templated char-array argument.
If we are to return a `std::array` I think we need to take the size as a template argument I think. And no compiler seems to accept `consteval` user defined literals.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2295942259)
Added section on `std::array<std::byte>` to PR summary.
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2295942259)
Added section on `std::array<std::byte>` to PR summary.
💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721392784)
> once we have full std::array<std::byte> support in CSscript:
Valid point.
----
What about repeated values, which are already using the vector constructor, i.e.
```C++
valtype(36, 0xff)
```
instead of
```C++
Vec(HexLiteral<uint8_t>("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"))
```
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721392784)
> once we have full std::array<std::byte> support in CSscript:
Valid point.
----
What about repeated values, which are already using the vector constructor, i.e.
```C++
valtype(36, 0xff)
```
instead of
```C++
Vec(HexLiteral<uint8_t>("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"))
```
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2295963942)
I haven't reviewed the last two commits, because I think the test-only changes offers the least amount of benefits, while being the hardest to review, because the type is changed and thus one has to make sure the call graph is still the same.
I think it would be better to remember the commit and then just update the called sites to accept `std::byte` (and then use that as an excuse to change the tests one-by-one to use the new HexLiteral function). Otherwise, the tests will be changed again a
...
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2295963942)
I haven't reviewed the last two commits, because I think the test-only changes offers the least amount of benefits, while being the hardest to review, because the type is changed and thus one has to make sure the call graph is still the same.
I think it would be better to remember the commit and then just update the called sites to accept `std::byte` (and then use that as an excuse to change the tests one-by-one to use the new HexLiteral function). Otherwise, the tests will be changed again a
...
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721377340)
nit in 309d8b783109eb52a5596712b19210271c8f882e (and the next commit):
I know I've raised this before in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716680866, but I still think it would be cleaner to drop this function (and related changes) from this pull request, because:
* It isn't strictly needed, because replacing the test-only `ScriptFromHex` wasn't a direct goal of this pull
* It is incomplete, because it adds a bit of compile-time checking for the new paths using `
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721377340)
nit in 309d8b783109eb52a5596712b19210271c8f882e (and the next commit):
I know I've raised this before in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716680866, but I still think it would be cleaner to drop this function (and related changes) from this pull request, because:
* It isn't strictly needed, because replacing the test-only `ScriptFromHex` wasn't a direct goal of this pull
* It is incomplete, because it adds a bit of compile-time checking for the new paths using `
...