Refactor ProcessNewBlock to reduce code duplication

rebroad commented at 11:47 pm on April 16, 2021: contributor

There are probably a few issues with this code (maybe there’s even a reason this code is duplicated as it currently is), so apologies in advance that I’m still a little (maybe very) bad with C++

rebroad force-pushed on Apr 16, 2021

DrahtBot added the label P2P on Apr 17, 2021

in src/net_processing.cpp:2314 in 89e5e5a679 outdated

2310@@ -2309,6 +2311,19 @@ void PeerManagerImpl::ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv)
2311     m_connman.PushMessage(&peer, std::move(msg));
2312 }
2313 
2314+void PeerManagerImpl::ProcessBlock(CNode& pfrom, const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock,

sipa commented at 3:52 am on April 17, 2021:

Nit: you can turn pblock into a const reference argument (const std::shared_ptr<const CBlock>& pblock), which will avoid an atomic shared_ptr copy at runtime.

practicalswift commented at 6:12 am on April 17, 2021:

chainparams is not being used in the function body. m_chainparams is used however.

rebroad commented at 8:09 am on April 17, 2021:

curious… I wonder why the tests aren’t failing then… I guess it inherits m_chainparams from the class, and therefore the parameter can be removed altogether from the function

MarcoFalke commented at 8:33 am on April 17, 2021:

I wonder why the tests aren’t failing then

We have no tests to detect unused parameters, IIRC

rebroad commented at 8:56 am on April 17, 2021:

Thanks. changed. If I may ask, why doesn’t ProcessNewBlock() also do this?

rebroad commented at 8:57 am on April 17, 2021:

I thought the compiler would at least give a warning - I’ve seen it warn about unused variables before, so I wonder why it doesn’t do this for unused parameters too.

MarcoFalke commented at 9:05 am on April 17, 2021:

Probably just a typo

sipa commented at 3:53 am on April 17, 2021: member

Looks obviously correct. utACK 89e5e5a67977588218c3c4309d9291b0cc5dd4ad

Refactor ProcessNewBlock to reduce code duplication 9a0653553a

rebroad force-pushed on Apr 17, 2021

in src/net_processing.cpp:3499 in 9a0653553a

3502-                pfrom.nLastBlockTime = GetTime();
3503-            } else {
3504-                LOCK(cs_main);
3505-                mapBlockSource.erase(pblock->GetHash());
3506-            }
3507+            ProcessBlock(pfrom, pblock, /*fForceProcessing=*/true);

promag commented at 9:08 am on April 17, 2021:

~~Why not just ProcessBlock(pfrom, pblock)?~~ nevermind.

promag commented at 9:11 am on April 17, 2021: member

Code review ACK 9a0653553a0ec403b4e7c6713466e0c7fa10ec94.

DrahtBot commented at 10:29 am on April 17, 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:

#21515 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)

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.

theStack approved

theStack commented at 11:04 pm on April 17, 2021: member

Code-review ACK 9a0653553a0ec403b4e7c6713466e0c7fa10ec94 🌴

MarcoFalke commented at 7:01 am on April 19, 2021: member

ACK 9a0653553a0ec403b4e7c6713466e0c7fa10ec94 💻

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 9a0653553a0ec403b4e7c6713466e0c7fa10ec94 💻
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiYnAv/Q8in67v6vQkKBao81ya/jLVKWU/9q7/X3Hpcvsqy4eD0zr+dr/sVG2Yw
 8YBDoA2Owy9M2Y0e+gEBOzQmYfwgsMeeotFaRrZfFdUCrxQ9LYcZlU93FlNcdJEFk
 9NlbQwUlmuQ8PQ2ZuDoJQetkGvROh0SscH4F755GDxP3PUoG4FoE3rkJdvIucQi6M
10LNH1kAcOQua8ftWJsrIN/tmnP4NFj5MM+baqwbd4rpPUE967/jjAPF1mrwOFJa/c
11orRnm65vi9MIQsq1zCJDv0WOxFlTnMZCcNy743/oClnklAyxByLxaNOkpSSb1tz1
12nZxiLw3ApRkIRxuZpgJUdCfNmLFbI3wfoU0xzL3bP84AGnUocyVyc2iFPRZcPLW8
135OXzzUowvVCIRa4Nn3bv7bHbwzhEJjKa0scm8+7wzX8VQjC1EmwHCJ1uvbU3I4vJ
14kNE2+XgSlNA9vEqzxVn6QiioiI/gfO8RT0svmrr+tcpBbVnTrqQ5aDUW1+KGNR2/
15lVlWmxbz
16=4Pl6
17-----END PGP SIGNATURE-----

Timestamp of file with hash a6152b70c0c6ef60deb64a857a8b598107e67ea657a7702b710dc476876d1c76 -

MarcoFalke merged this on Apr 19, 2021

MarcoFalke closed this on Apr 19, 2021

fanquake commented at 7:37 am on April 19, 2021: member

Note that this PR changed the file permissions of src/net_processsing.cpp for no reason.

in src/net_processing.cpp:454 in 9a0653553a

450@@ -451,6 +451,8 @@ class PeerManagerImpl final : public PeerManager
451 
452     void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(peer.m_getdata_requests_mutex) LOCKS_EXCLUDED(::cs_main);
453 
454+    void ProcessBlock(CNode& pfrom, const std::shared_ptr<const CBlock>& pblock, bool fForceProcessing);

jnewbery commented at 11:54 am on April 19, 2021:

The project style is to not use hungarian notation, and use snake case for parameter/variable names:

0    void ProcessBlock(CNode& node, const std::shared_ptr<const CBlock>& block, bool force_processing);

jnewbery commented at 12:05 pm on April 19, 2021:

New functions should have doxygen comments.

in src/net_processing.cpp:2316 in 9a0653553a

2310@@ -2309,6 +2311,18 @@ void PeerManagerImpl::ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv)
2311     m_connman.PushMessage(&peer, std::move(msg));
2312 }
2313 
2314+void PeerManagerImpl::ProcessBlock(CNode& pfrom, const std::shared_ptr<const CBlock>& pblock, bool fForceProcessing)
2315+{
2316+    bool fNewBlock = false;

jnewbery commented at 12:05 pm on April 19, 2021:

0    bool new_block{false};

jnewbery commented at 12:10 pm on April 19, 2021: member

Functionality looks correct. It would have been nice to follow the style guide for the new code.

sidhujag referenced this in commit d52bf340ce on Apr 19, 2021

fanquake referenced this in commit 67a359313f on Apr 20, 2021

sidhujag referenced this in commit e103e1eb7a on Apr 20, 2021

fanquake referenced this in commit 0a3b8ea11a on Jun 2, 2021

sidhujag referenced this in commit 7b8dd30fd9 on Jun 3, 2021

MarcoFalke deleted a comment on Apr 7, 2022

MarcoFalke locked this on Apr 7, 2022

Refactor ProcessNewBlock to reduce code duplication #21713

Conflicts