💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1862981292)
I moved chunking into `noise.h`, since the maximum message length of 65 KB is part of the protocol. I also dropped `ProcessMsg` handler in favor of handling handshake vs. regular transport more directly. Finally `SendBuf` is now called by `EncryptAndSendMessage` rather than by each message call site.
Main changes:
* 945838fca264c8469e24d629a4552741dbbd4dda -> f08a10a892fc467dbf6037e0c73227822e9f698a
* fc0aa335dd3f3beba383a42f19f73822b4ad2d34 -> 028c286d9a38343d8240c62a67207681d024a326
*
...
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1862981292)
I moved chunking into `noise.h`, since the maximum message length of 65 KB is part of the protocol. I also dropped `ProcessMsg` handler in favor of handling handshake vs. regular transport more directly. Finally `SendBuf` is now called by `EncryptAndSendMessage` rather than by each message call site.
Main changes:
* 945838fca264c8469e24d629a4552741dbbd4dda -> f08a10a892fc467dbf6037e0c73227822e9f698a
* fc0aa335dd3f3beba383a42f19f73822b4ad2d34 -> 028c286d9a38343d8240c62a67207681d024a326
*
...
💬 EthnTuttle commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1862983648)
Ack.
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1862983648)
Ack.
📝 maflcko opened a pull request: "util: Faster std::byte (pre)vector (un)serialize"
(https://github.com/bitcoin/bitcoin/pull/29114)
Currently, large vectors of `std::byte` are (un)serialized byte-by-byte, which is slow. Fix this, by enabling the already existing optimization for them.
On my system this gives a 10x speedup for `./src/bench/bench_bitcoin --filter=PrevectorDeserializeTrivial`, when `std::byte` are used:
```diff
diff --git a/src/bench/prevector.cpp b/src/bench/prevector.cpp
index 2524e215e4..76b16bc34e 100644
--- a/src/bench/prevector.cpp
+++ b/src/bench/prevector.cpp
@@ -17,7 +17,7 @@ struct nontrivi
...
(https://github.com/bitcoin/bitcoin/pull/29114)
Currently, large vectors of `std::byte` are (un)serialized byte-by-byte, which is slow. Fix this, by enabling the already existing optimization for them.
On my system this gives a 10x speedup for `./src/bench/bench_bitcoin --filter=PrevectorDeserializeTrivial`, when `std::byte` are used:
```diff
diff --git a/src/bench/prevector.cpp b/src/bench/prevector.cpp
index 2524e215e4..76b16bc34e 100644
--- a/src/bench/prevector.cpp
+++ b/src/bench/prevector.cpp
@@ -17,7 +17,7 @@ struct nontrivi
...
💬 maflcko commented on pull request "util: Faster std::byte (pre)vector (un)serialize":
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1862986532)
Draft while it is based on https://github.com/bitcoin/bitcoin/pull/29056
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1862986532)
Draft while it is based on https://github.com/bitcoin/bitcoin/pull/29056
👍 TheCharlatan approved a pull request: "refactor: Print verbose serialize compiler error messages"
(https://github.com/bitcoin/bitcoin/pull/29056#pullrequestreview-1789197585)
Re-ACK fae526345de539ab8f9b80100f6dfbe8e1d3284b
(https://github.com/bitcoin/bitcoin/pull/29056#pullrequestreview-1789197585)
Re-ACK fae526345de539ab8f9b80100f6dfbe8e1d3284b
📝 ismaelsadeeq opened a pull request: "[doc]: add doxygen comment describing what `CheckPackageLimits` returns"
(https://github.com/bitcoin/bitcoin/pull/29115)
This PR added a doxygen comment on `CheckPackageLimits` describing what the method returns.
Fixes https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1429805433
(https://github.com/bitcoin/bitcoin/pull/29115)
This PR added a doxygen comment on `CheckPackageLimits` describing what the method returns.
Fixes https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1429805433
💬 ismaelsadeeq commented on pull request "wallet, mempool: propagete `checkChainLimits` error message to wallet":
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1431646003)
This is fixed in https://github.com/bitcoin/bitcoin/pull/29115
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1431646003)
This is fixed in https://github.com/bitcoin/bitcoin/pull/29115
💬 jamesob commented on pull request "util: Faster std::byte (pre)vector (un)serialize":
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1863092993)
Concept ACK. I better blow the dust off of bitcoinperf...
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1863092993)
Concept ACK. I better blow the dust off of bitcoinperf...
🤔 ryanofsky reviewed a pull request: "doc: Add multiprocess design doc"
(https://github.com/bitcoin/bitcoin/pull/28978#pullrequestreview-1788797844)
re: https://github.com/bitcoin/bitcoin/pull/28978#pullrequestreview-1777912181
> Approach ACK. I'm still working on my understanding of multiprocess but this document was very helpful already in its current state, no major suggestions at this point.
Thank you, applied your suggestions, and improved the example flow section based on your feedback.
re: https://github.com/bitcoin/bitcoin/pull/28978#pullrequestreview-1782842554
> ACK [837c53d](https://github.com/bitcoin/bitcoin/commit/83
...
(https://github.com/bitcoin/bitcoin/pull/28978#pullrequestreview-1788797844)
re: https://github.com/bitcoin/bitcoin/pull/28978#pullrequestreview-1777912181
> Approach ACK. I'm still working on my understanding of multiprocess but this document was very helpful already in its current state, no major suggestions at this point.
Thank you, applied your suggestions, and improved the example flow section based on your feedback.
re: https://github.com/bitcoin/bitcoin/pull/28978#pullrequestreview-1782842554
> ACK [837c53d](https://github.com/bitcoin/bitcoin/commit/83
...
💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361667)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1424253642
> Steps 2 and 3 highlight what's handled by generated code, for consistency that might be helpful in steps 4&5 too?
Thanks, added more details to this section so it now mentions the generated client and server classes and references the generated code consistently.
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361667)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1424253642
> Steps 2 and 3 highlight what's handled by generated code, for consistency that might be helpful in steps 4&5 too?
Thanks, added more details to this section so it now mentions the generated client and server classes and references the generated code consistently.
💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361020)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1424248273
> typo nit
Thanks, fixed
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361020)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1424248273
> typo nit
Thanks, fixed
💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431360695)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1427363274
> nit
Useful clarification, added
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431360695)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1427363274
> nit
Useful clarification, added
💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361157)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428164608
>
Thanks, fixed
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361157)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428164608
>
Thanks, fixed
💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361546)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1424249827
> nit: rogue backtick
Thanks, fixed
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361546)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1424249827
> nit: rogue backtick
Thanks, fixed
💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431360934)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428162298
> nit: typo
Thanks, fixed
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431360934)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428162298
> nit: typo
Thanks, fixed
💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361251)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428165515
>
Thanks, fixed
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361251)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428165515
>
Thanks, fixed
💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361433)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428187716
> Maybe mention that his is based on code that is not merged yet, for example `chain.capnp` is linked below but it doesn't exist on master yet.
Oops, this is not good. I changed the link to point to the branch version for now since I think seeing the interface definition can be helpful.
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361433)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428187716
> Maybe mention that his is based on code that is not merged yet, for example `chain.capnp` is linked below but it doesn't exist on master yet.
Oops, this is not good. I changed the link to point to the branch version for now since I think seeing the interface definition can be helpful.
💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431360480)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1427365136
> nit suggestions:
>
>* 'central to the bitcoin network' isn't really relevant to this doc
>
>* I hear people complain that big changes are always a security risk in the short term all the time
Yes, definitely agree and took the suggestion.
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431360480)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1427365136
> nit suggestions:
>
>* 'central to the bitcoin network' isn't really relevant to this doc
>
>* I hear people complain that big changes are always a security risk in the short term all the time
Yes, definitely agree and took the suggestion.
💬 maflcko commented on pull request "util: Faster std::byte (pre)vector (un)serialize":
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1863110548)
Benchmarks are also done by https://corecheck.dev/bitcoin/bitcoin/pulls/29114 :)
Though, to test this, one would have to manually apply the diff either way, so local running is probably the easiest.
(I don't think `std::vector<std::byte>` serialization is used anywhere, where performance matters or is measured)
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1863110548)
Benchmarks are also done by https://corecheck.dev/bitcoin/bitcoin/pulls/29114 :)
Though, to test this, one would have to manually apply the diff either way, so local running is probably the easiest.
(I don't think `std::vector<std::byte>` serialization is used anywhere, where performance matters or is measured)
👍 ismaelsadeeq approved a pull request: "Add multiplication operator to CFeeRate"
(https://github.com/bitcoin/bitcoin/pull/29037#pullrequestreview-1789316545)
ACK 1757452cc55a6dacc62e4258043ee4d711fd281a
(https://github.com/bitcoin/bitcoin/pull/29037#pullrequestreview-1789316545)
ACK 1757452cc55a6dacc62e4258043ee4d711fd281a