Refactor ProcessNewBlock to reduce code duplication #21713

rebroad · 2021-04-16T23:47:14Z

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++

sipa · 2021-04-17T03:52:40Z

src/net_processing.cpp

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.

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

Probably just a typo

sipa · 2021-04-17T03:53:15Z

Looks obviously correct. utACK 89e5e5a67977588218c3c4309d9291b0cc5dd4ad

practicalswift · 2021-04-17T06:12:34Z

src/net_processing.cpp

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

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

I wonder why the tests aren't failing then

We have no tests to detect unused parameters, IIRC

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.

promag · 2021-04-17T09:08:19Z

src/net_processing.cpp

-                LOCK(cs_main);
-                mapBlockSource.erase(pblock->GetHash());
-            }
+            ProcessBlock(pfrom, pblock, /*fForceProcessing=*/true);


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

promag

Code review ACK 9a06535.

DrahtBot · 2021-04-17T10:29:21Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Erlay: bandwidth-efficient transaction relay protocol #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

Code-review ACK 9a06535 🌴

maflcko · 2021-04-19T07:01:29Z

ACK 9a06535 💻

Show signature and timestamp

Signature:

Timestamp of file with hash a6152b70c0c6ef60deb64a857a8b598107e67ea657a7702b710dc476876d1c76 -

fanquake · 2021-04-19T07:37:47Z

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

jnewbery

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

jnewbery · 2021-04-19T11:54:10Z

src/net_processing.cpp


    void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(peer.m_getdata_requests_mutex) LOCKS_EXCLUDED(::cs_main);

+    void ProcessBlock(CNode& pfrom, const std::shared_ptr<const CBlock>& pblock, bool fForceProcessing);


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

Suggested change

void ProcessBlock(CNode& pfrom, const std::shared_ptr<const CBlock>& pblock, bool fForceProcessing);

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

jnewbery · 2021-04-19T12:05:26Z

src/net_processing.cpp


    void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(peer.m_getdata_requests_mutex) LOCKS_EXCLUDED(::cs_main);

+    void ProcessBlock(CNode& pfrom, const std::shared_ptr<const CBlock>& pblock, bool fForceProcessing);


New functions should have doxygen comments.

jnewbery · 2021-04-19T12:05:55Z

src/net_processing.cpp


+void PeerManagerImpl::ProcessBlock(CNode& pfrom, const std::shared_ptr<const CBlock>& pblock, bool fForceProcessing)
+{
+    bool fNewBlock = false;


Suggested change

bool fNewBlock = false;

bool new_block{false};

9a06535 Refactor ProcessNewBlock to reduce code duplication (R E Broadley) Pull request description: 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++ ACKs for top commit: MarcoFalke: ACK 9a06535 💻 promag: Code review ACK 9a06535. theStack: Code-review ACK 9a06535 🌴 Tree-SHA512: f8634ffad4b2370204d1a0945db4e27248b9e579d9912784da432b8ee3303cae424fa9f7500000dcfb31e6d29d04a8f7d322d17a6fe3d4adaddd10c539458a8c

f2f2541 remove executable flag for src/net_processing.cpp (Sebastian Falbesoner) Pull request description: The file permissions for `src/net_processing.cpp` have been changed in #21713, as discovered by fanquake (bitcoin/bitcoin#21713 (comment)). This PR removes the executable flag again. ACKs for top commit: kiminuo: ACK f2f2541 :) jnewbery: ACK f2f2541 promag: ACK f2f2541. Tree-SHA512: 1d5a62afb1152029e69fccea2ae53dcb262a91724a5c03dfc4de8c409b280814d0c211c2f9a71f1a6e927f4ed571ba4ac311de9de8ebb797eaf1051674241bdb

f2f2541 remove executable flag for src/net_processing.cpp (Sebastian Falbesoner) Pull request description: The file permissions for `src/net_processing.cpp` have been changed in bitcoin#21713, as discovered by fanquake (bitcoin#21713 (comment)). This PR removes the executable flag again. ACKs for top commit: kiminuo: ACK f2f2541 :) jnewbery: ACK f2f2541 promag: ACK f2f2541. Tree-SHA512: 1d5a62afb1152029e69fccea2ae53dcb262a91724a5c03dfc4de8c409b280814d0c211c2f9a71f1a6e927f4ed571ba4ac311de9de8ebb797eaf1051674241bdb

e12f287 net: cleanup newly added PeerManagerImpl::ProcessNewBlock (fanquake) 610151f validation: change ProcessNewBlock() to take a CBlock reference (fanquake) Pull request description: Addresses some [post-merge comments](#21713 (review)) from #21713. Also makes `ChainstateManager::ProcessNewBlock` take a const reference argument, as it [was asked](#21713 (comment)) why it was not the case in that PR. ACKs for top commit: jnewbery: Code review ACK e12f287 MarcoFalke: review ACK e12f287 🚚 Tree-SHA512: 9c3e7353240c862d50bce2a0f58741c109dd628040b56ed46250103f8ebe9009238b131da710486791e28e3a83c985057b7be0a32aed1a929269b43097c7425b

…itcoin#21713 e12f287 net: cleanup newly added PeerManagerImpl::ProcessNewBlock (fanquake) 610151f validation: change ProcessNewBlock() to take a CBlock reference (fanquake) Pull request description: Addresses some [post-merge comments](bitcoin#21713 (review)) from bitcoin#21713. Also makes `ChainstateManager::ProcessNewBlock` take a const reference argument, as it [was asked](bitcoin#21713 (comment)) why it was not the case in that PR. ACKs for top commit: jnewbery: Code review ACK e12f287 MarcoFalke: review ACK e12f287 🚚 Tree-SHA512: 9c3e7353240c862d50bce2a0f58741c109dd628040b56ed46250103f8ebe9009238b131da710486791e28e3a83c985057b7be0a32aed1a929269b43097c7425b

rebroad force-pushed the RefactorProcessNewBlock branch from 3e24a66 to 89e5e5a Compare April 16, 2021 23:49

DrahtBot added the P2P label Apr 17, 2021

sipa reviewed Apr 17, 2021

View reviewed changes

practicalswift reviewed Apr 17, 2021

View reviewed changes

Refactor ProcessNewBlock to reduce code duplication

9a06535

rebroad force-pushed the RefactorProcessNewBlock branch from 89e5e5a to 9a06535 Compare April 17, 2021 08:55

promag reviewed Apr 17, 2021

View reviewed changes

DrahtBot mentioned this pull request Apr 17, 2021

Erlay: bandwidth-efficient transaction relay protocol #21515

Closed

theStack approved these changes Apr 17, 2021

View reviewed changes

maflcko merged commit 17b51cd into bitcoin:master Apr 19, 2021

theStack mentioned this pull request Apr 19, 2021

remove executable flag for src/net_processing.cpp #21728

Merged

jnewbery reviewed Apr 19, 2021

View reviewed changes

jamesob mentioned this pull request May 27, 2021

assumeutxo #15606

Closed

18 tasks

fanquake mentioned this pull request May 31, 2021

refactor: address ProcessNewBlock comments from #21713 #22106

Merged

This was referenced May 31, 2021

refactor: Remove ::Params() global from CChainState #21789

Merged

[doc] explain why CheckBlock() is called before AcceptBlock #15545

Merged

bitcoin deleted a comment from arslan-raza-143 Apr 7, 2022

bitcoin locked and limited conversation to collaborators Apr 7, 2022


		void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(peer.m_getdata_requests_mutex) LOCKS_EXCLUDED(::cs_main);

		void ProcessBlock(CNode& pfrom, const std::shared_ptr<const CBlock>& pblock, bool fForceProcessing);

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

Refactor ProcessNewBlock to reduce code duplication #21713

Refactor ProcessNewBlock to reduce code duplication #21713

Uh oh!

Conversation

rebroad commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Apr 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rebroad Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Apr 17, 2021

Conflicts

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 19, 2021

Uh oh!

fanquake commented Apr 19, 2021

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

rebroad commented Apr 16, 2021 •

edited

Loading

rebroad Apr 17, 2021 •

edited

Loading

promag Apr 17, 2021 •

edited

Loading

jnewbery Apr 19, 2021 •

edited

Loading