p2p: Serialize cmpctblock at most once in NewPoWValidBlock #23880

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2112-p2pAsync changing 1 files +7 −3

MarcoFalke commented at 5:12 PM on December 27, 2021: member

Instead of serializing for each peer, serialize at most once and copy the raw data for each peer.
MarcoFalke added the label Refactoring on Dec 27, 2021
MarcoFalke added the label P2P on Dec 27, 2021
theStack commented at 5:20 PM on December 27, 2021: member

Concept ACK

IIRC I also tried to tackle this TODO some time ago but failed as move-semantics prevented to store the serialized message. This PR is now a good opportunity to learn about futures and std::async.
MarcoFalke commented at 5:27 PM on December 27, 2021: member

Yeah, it is not possible to unconditionally std::move the result of a std::shared_future here, since it is unknown if there is another call to the getter later on. (In theory it might be possible to avoid the last copy and do a move instead, but I am not sure if this is worth it, let alone easily possible).
DrahtBot commented at 5:07 AM on December 28, 2021: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #20799 (net processing: Only support version 2 compact blocks by jnewbery)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
shaavan approved
shaavan commented at 2:20 PM on December 28, 2021: contributor

ACK fabe7843a767505abdc580581e330fb4f963c788

I was researching about this PR. I found this useful article. It talks about how using std::deferred allows lazy calculations. If the value has been already calculated, it will not be recalculated, potentially decreasing the number of times serialization is done to one.

I have reviewed the code changes in this PR, and I found them agreeable.

in src/net_processing.cpp:1597 in fabe7843a7 outdated

1592 | @@ -1591,7 +1593,9 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha
1593 |  
1594 |              LogPrint(BCLog::NET, "%s sending header-and-ids %s to peer=%d\n", "PeerManager::NewPoWValidBlock",
1595 |                      hashBlock.ToString(), pnode->GetId());
1596 | -            m_connman.PushMessage(pnode, msgMaker.Make(NetMsgType::CMPCTBLOCK, *pcmpctblock));
1597 | +
1598 | +            const CSerializedNetMsg& ser_cpmptblock{lazy_ser.get()};

theStack commented at 6:40 PM on January 24, 2022:

This is a typo I guess?

            const CSerializedNetMsg& ser_cmpctblock{lazy_ser.get()};

MarcoFalke commented at 8:19 AM on January 26, 2022:

Thx, fixed

p2p: Serialize cmpctblock at most once in NewPoWValidBlock fa61dd44f9
MarcoFalke force-pushed on Jan 26, 2022
shaavan approved
shaavan commented at 12:22 PM on January 28, 2022: contributor
reACK fa61dd44f99323d10b0122c13224bc5cdb5e3d2a

Changes since my last review:
- Fixed a typo. ser_cpmptblock -> ser_cmpctblock.
- Rebased over current master.
theStack approved
theStack commented at 1:58 PM on January 30, 2022: member

After doing some more research about the concepts of futures and lazy evaluation in C++, I'm convinced now that this change is correct. This seems to be the first time ever that std::async and std::shared_future are used in the Bitcoin Core codebase.

Code-review ACK fa61dd44f99323d10b0122c13224bc5cdb5e3d2a
MarcoFalke merged this on Mar 21, 2022
MarcoFalke closed this on Mar 21, 2022
MarcoFalke deleted the branch on Mar 21, 2022
jonatack commented at 1:09 PM on March 21, 2022: member

Nice. Posthumous ACK.
sidhujag referenced this in commit 1afced4649 on Mar 22, 2022
jnewbery commented at 12:19 PM on March 25, 2022: member
Concept ACK.

Is there a reason you need a std::shared_future rather than a std::future? It never gets copied for usage by multiple threads, which I believe is the usage for a shared_future. Would this be equivalent to what you've used here:
```
-    const std::shared_future<CSerializedNetMsg> lazy_ser{
+    std::future<CSerializedNetMsg> lazy_ser{
```
Could the same future be used to replace the most_recent_compact_block global, so that we don't have to reserialize the compact block for peers that request the compact block in low bandwidth mode?
MarcoFalke commented at 12:27 PM on March 25, 2022: member

No, that'd be UB, see https://en.cppreference.com/w/cpp/thread/future/get

valid() is false after a call to std::future<T>::get() The behavior is undefined if valid() is false before the call to get()
MarcoFalke commented at 12:28 PM on March 25, 2022: member

Could the same future be used to replace the most_recent_compact_block global, so that we don't have to reserialize the compact block for peers that request the compact block in low bandwidth mode?

Likely, but I haven't checked.
jnewbery commented at 2:40 PM on March 25, 2022: member

No, that'd be UB, see en.cppreference.com/w/cpp/thread/future/get

Ah! Thanks.
MarcoFalke commented at 3:30 PM on April 1, 2022: member

For reference, for anyone wondering: A copy from the lazy ser is 25% faster on my machine than serialize.
DrahtBot locked this on Apr 1, 2023

Contributors

Labels

Linked (view graph)

#26755 p2p: cache compact block message to use for low bandwidth CMPCT_BLOCK

p2p: Serialize cmpctblock at most once in NewPoWValidBlock #23880

Conflicts