✅ jonatack closed a pull request: "rpc, cli: return "verificationprogress" of 1 when up to date"
(https://github.com/bitcoin/bitcoin/pull/31135)
(https://github.com/bitcoin/bitcoin/pull/31135)
💬 jonatack commented on pull request "rpc, cli: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2436139570)
I agree with the approaches suggested by @sipa and @laanwj above and will be happy to review them. However, there are no concept ACKs and I am not confident of garnering sufficient ACKs if I implement those approaches here myself, so leaving it up for grabs.
(https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2436139570)
I agree with the approaches suggested by @sipa and @laanwj above and will be happy to review them. However, there are no concept ACKs and I am not confident of garnering sufficient ACKs if I implement those approaches here myself, so leaving it up for grabs.
📝 instagibbs opened a pull request: "functional test: Additional package evaluation coverage"
(https://github.com/bitcoin/bitcoin/pull/31152)
Current test coverage doesn't ensure that mempool trimming doesn't appear prior to the entire package, and not just the subpackage, is finished being submitted.
Add a scenario that covers this case, where package ancestors can make it in individually, but would be immadiately evicted if not for the package CPFP.
in response to https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813272637 where if applied onto that PR, the test fails due to package failure.
(https://github.com/bitcoin/bitcoin/pull/31152)
Current test coverage doesn't ensure that mempool trimming doesn't appear prior to the entire package, and not just the subpackage, is finished being submitted.
Add a scenario that covers this case, where package ancestors can make it in individually, but would be immadiately evicted if not for the package CPFP.
in response to https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813272637 where if applied onto that PR, the test fails due to package failure.
💬 maflcko commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2436158760)
It would be good to explain the jpegs in the description, or even remove them. They will be excluded from the merge commit and aren't shown, unless GitHub happens to be reachable and online. Are they saying that IBD was 4% faster? Also, I think they were created with the UB version of this pull, so may be outdated either way?
I did a quick check on my laptop and it seems the `XorSmall` (1+4 bytes) is slower with this pull. The `Xor` (modified to check 40 bytes) was twice as fast. Overall, I'd
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2436158760)
It would be good to explain the jpegs in the description, or even remove them. They will be excluded from the merge commit and aren't shown, unless GitHub happens to be reachable and online. Are they saying that IBD was 4% faster? Also, I think they were created with the UB version of this pull, so may be outdated either way?
I did a quick check on my laptop and it seems the `XorSmall` (1+4 bytes) is slower with this pull. The `Xor` (modified to check 40 bytes) was twice as fast. Overall, I'd
...
💬 maflcko commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1815599531)
> Maybe this can be documented in `doc/developer-notes.md`?
(An alternative would be to add the CI configs to the cmake presets file, because the ISan is part of one. )
(https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1815599531)
> Maybe this can be documented in `doc/developer-notes.md`?
(An alternative would be to add the CI configs to the cmake presets file, because the ISan is part of one. )
💬 maflcko commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1815601596)
Yes. At least for me locally, but I am getting widely different bench results anyway: https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2436158760
With this one reverted, `XorSmall` is back to being just slightly slower than current master for me.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1815601596)
Yes. At least for me locally, but I am getting widely different bench results anyway: https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2436158760
With this one reverted, `XorSmall` is back to being just slightly slower than current master for me.
💬 edilmedeiros commented on pull request "rpc, cli: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2436213630)
Maybe #31127 might be marked as Good First Issue?
(https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2436213630)
Maybe #31127 might be marked as Good First Issue?
👍 hodlinator approved a pull request: "tinyformat: enforce compile-time checks for format string literals"
(https://github.com/bitcoin/bitcoin/pull/31149#pullrequestreview-2393618400)
ACK 4cf12216951e91550c81db807fe0ecfafe6834c9
Only dropped commit I had [concerns about](https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2431257072). [l0rinc too](https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2389270316).
Does not entirely prevent throwing of exceptions from incorrect format strings (notably translated strings), but is a step in the right direction (validating more format strings at compile time).
(https://github.com/bitcoin/bitcoin/pull/31149#pullrequestreview-2393618400)
ACK 4cf12216951e91550c81db807fe0ecfafe6834c9
Only dropped commit I had [concerns about](https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2431257072). [l0rinc too](https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2389270316).
Does not entirely prevent throwing of exceptions from incorrect format strings (notably translated strings), but is a step in the right direction (validating more format strings at compile time).
💬 hodlinator commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2436261655)
> > Worth bringing back the try/catch in src/test/fuzz/strprintf.cpp for Debug builds?
>
> We should definitely be able to see easily what the invalid values were (and add unit tests for those cases)
(Not sure that would add much value, we know the fuzzing will produce mostly garbage format strings that will trigger errors).
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2436261655)
> > Worth bringing back the try/catch in src/test/fuzz/strprintf.cpp for Debug builds?
>
> We should definitely be able to see easily what the invalid values were (and add unit tests for those cases)
(Not sure that would add much value, we know the fuzzing will produce mostly garbage format strings that will trigger errors).
💬 maflcko commented on pull request "doc: replace `-?` with `-h` and `-help`":
(https://github.com/bitcoin/bitcoin/pull/31118#issuecomment-2436264728)
lgtm ACK 33a28e252a7349c0aa284005aee97873b965fcfe
(https://github.com/bitcoin/bitcoin/pull/31118#issuecomment-2436264728)
lgtm ACK 33a28e252a7349c0aa284005aee97873b965fcfe
💬 l0rinc commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1815667257)
Usually these optimizations concentrate on the measurable parts based on the profiling results that I'm getting during reindexing or IBD. Obfuscating a single bit (i.e. `XorSmall`) wasn't my focus, it's already very fast, didn't seem like the bottleneck.
Would you like me to concentrate on that scenario instead? Or would it make more sense to serialize a block and use that as the basis for the benchmarks?
> C++ compiler .......................... AppleClang 16.0.0.16000026
Before:
|
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1815667257)
Usually these optimizations concentrate on the measurable parts based on the profiling results that I'm getting during reindexing or IBD. Obfuscating a single bit (i.e. `XorSmall`) wasn't my focus, it's already very fast, didn't seem like the bottleneck.
Would you like me to concentrate on that scenario instead? Or would it make more sense to serialize a block and use that as the basis for the benchmarks?
> C++ compiler .......................... AppleClang 16.0.0.16000026
Before:
|
...
💬 l0rinc commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2436298046)
> Are they saying that IBD was 4% faster?
That's what I'm measuring currently, but I don't expect more than 2% difference here.
Posting the perf here for reference:
<img width="500" alt="image" src="https://github.com/user-attachments/assets/6dc99dee-bda8-40d0-bdb0-2030982e0645">
Reindexing until 300k blocks reveals that XOR usage was reduced:
<img width="500" alt="image" src="https://github.com/user-attachments/assets/fe308c34-da65-42f3-9b10-7d34aa9a8056">
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2436298046)
> Are they saying that IBD was 4% faster?
That's what I'm measuring currently, but I don't expect more than 2% difference here.
Posting the perf here for reference:
<img width="500" alt="image" src="https://github.com/user-attachments/assets/6dc99dee-bda8-40d0-bdb0-2030982e0645">
Reindexing until 300k blocks reveals that XOR usage was reduced:
<img width="500" alt="image" src="https://github.com/user-attachments/assets/fe308c34-da65-42f3-9b10-7d34aa9a8056">
💬 maflcko commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1815699568)
> Would you like me to concentrate on that scenario instead? Or would it make more sense to serialize a block and use that as the basis for the benchmarks?
Well no. I think this has been mentioned previously. Generally, optimizing for micro benchmarks may not yield results that are actually meaningful or visible for end-users, because the benchmarks capture only a very specific and narrow view. Optimizing for one could even make the code slower for another (as observed above). Adding a bench
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1815699568)
> Would you like me to concentrate on that scenario instead? Or would it make more sense to serialize a block and use that as the basis for the benchmarks?
Well no. I think this has been mentioned previously. Generally, optimizing for micro benchmarks may not yield results that are actually meaningful or visible for end-users, because the benchmarks capture only a very specific and narrow view. Optimizing for one could even make the code slower for another (as observed above). Adding a bench
...
💬 mzumsande commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1815522185)
same as 2 lines above
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1815522185)
same as 2 lines above
🤔 mzumsande reviewed a pull request: "test: extend the SOCKS5 Python proxy to actually connect to a destination"
(https://github.com/bitcoin/bitcoin/pull/29420#pullrequestreview-2393420379)
Code review / tested ACK 57529ac4dbb2721c1ad0a3566f0299dbdb5ca5c0
This is pretty cool, and could be used in other places than just for the private transaction relay (e.g. whenever we'd like to have an onion outbound peer in a functional test and exchange messages with it)
I tested this by extending `feature_anchors.py` to use a `destinations_factory` and checked that it completes the version handshake and behaves like a normal connection.
(https://github.com/bitcoin/bitcoin/pull/29420#pullrequestreview-2393420379)
Code review / tested ACK 57529ac4dbb2721c1ad0a3566f0299dbdb5ca5c0
This is pretty cool, and could be used in other places than just for the private transaction relay (e.g. whenever we'd like to have an onion outbound peer in a functional test and exchange messages with it)
I tested this by extending `feature_anchors.py` to use a `destinations_factory` and checked that it completes the version handshake and behaves like a normal connection.
💬 mzumsande commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1815521913)
This log isn't accurate if `keep_alive` is set, because then we'll log that we close the connection but actually stay connected.
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1815521913)
This log isn't accurate if `keep_alive` is set, because then we'll log that we close the connection but actually stay connected.
💬 achow101 commented on pull request "doc: replace `-?` with `-h` and `-help`":
(https://github.com/bitcoin/bitcoin/pull/31118#issuecomment-2436408742)
ACK 33a28e252a7349c0aa284005aee97873b965fcfe
(https://github.com/bitcoin/bitcoin/pull/31118#issuecomment-2436408742)
ACK 33a28e252a7349c0aa284005aee97873b965fcfe
🤔 hodlinator reviewed a pull request: "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior"
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2393651994)
Concept ACK a1c9d23ff587b84d37175b2993ea6760f9762177
Good to fix weird negation corner cases.
(Very tempting to slap `DISALLOW_NEGATION` on non-list args for some reason, but I guess someone could be using `norpcwallet` to reset earlier values instead of `rpcwallet=`).
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2393651994)
Concept ACK a1c9d23ff587b84d37175b2993ea6760f9762177
Good to fix weird negation corner cases.
(Very tempting to slap `DISALLOW_NEGATION` on non-list args for some reason, but I guess someone could be using `norpcwallet` to reset earlier values instead of `rpcwallet=`).
💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1815689215)
Better to explain *why* than *what* (log message already explains *what*). To me it's not immediately obvious that we should avoid using DNS seeds when having a Tor SOCKS5 proxy?
Furthermore, as I understand it we don't really ignore `-dnsseed` when having a proxy; we use them differently, making the log message not correct to begin with?:
https://github.com/bitcoin/bitcoin/blob/a1c9d23ff587b84d37175b2993ea6760f9762177/src/net.cpp#L2284-L2288
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1815689215)
Better to explain *why* than *what* (log message already explains *what*). To me it's not immediately obvious that we should avoid using DNS seeds when having a Tor SOCKS5 proxy?
Furthermore, as I understand it we don't really ignore `-dnsseed` when having a proxy; we use them differently, making the log message not correct to begin with?:
https://github.com/bitcoin/bitcoin/blob/a1c9d23ff587b84d37175b2993ea6760f9762177/src/net.cpp#L2284-L2288
💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1815722428)
More useful comment IMO:
```suggestion
// Populate outgoing connections unless negated or disabled.
if (connect.size() != 1 || connect[0] != "0") {
connOptions.m_specified_outgoing = connect;
}
```
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1815722428)
More useful comment IMO:
```suggestion
// Populate outgoing connections unless negated or disabled.
if (connect.size() != 1 || connect[0] != "0") {
connOptions.m_specified_outgoing = connect;
}
```