Nano-node: Useless if block

[gurghet]

https://github.com/nanocurrency/nano-node/blob/9112636c22458ceb5c2fe6cdfe7f80c76ccae924/nano/node/blockprocessor.cpp#L106-L124
Can we delete this please? It's just confusing. Both branches do the same thing and there is a lengthy explanation of something that it should be obvious but it's not.

[clemahieu]

https://github.com/nanocurrency/nano-node/commit/a96664c7cce49327e9df50d41cf830b7c48a20ff

[Srayman]

Is this correct? In V21.2/V21.3 it used push_back in the later section.

The intent here is that after a block is processed and it is retrieving successors from queue_unchecked it would push to the front of the queue and blocks through bootstrap would be pushed to the back of the queue therefore prioritizing which blocks get processed for those most likely to fit into the ledger as well as live blocks.

It seems like this commit inadvertently changed it to both be pushing to the front, guessing copy/paste error? https://github.com/nanocurrency/nano-node/commit/664a940df822845c94504d5306eaba09b5a1a666

[gurghet]

@Srayman I saw the commit and noticed that a new parameter was added. So it's probably as you said, a copy paste error. Still it would be best to have a clearer explanation in the comment since the behaviour is not at all obvious.

[Srayman]

Are you referring to this description in the comment in the code... pasted below?

Either way it certainly shouldn’t be emplace_front for all blocks, or bootstrap blocks and new blocks will go to the front and push older blocks to the back which is the opposite of how it should be processing blocks as they should be FIFO not LIFO.

    /* Push blocks from unchecked to front of processing deque to keep more operations with unchecked inside of single write transaction. 
    It's designed to help with realtime blocks traffic if block processor is not performing large task like bootstrap. 
    If deque is a quarter full then push back to allow other blocks processing. */

[gurghet]

Yeah, I think it mixes information about transactions and how the queue is used in other places on top of the fact that quarter_full is a magic number hidden behind a nice name. If I understand correctly it should be

else if (push_front_preference_a && low_load) {
  // push new block to the front of deque to help with realtime traffic
  nano::lock_guard<nano::mutex> guard (mutex);
  blocks.emplace_front (info_a, false);
  condition.notify_all ();
}
else
{
  // push new block to the back of deque to help with old block prioritization
  nano::lock_guard<nano::mutex> guard (mutex);
  blocks.emplace_back (info_a, false);
  condition.notify_all ();
}

This way there is no assumption on how the rest of the code works and the comments will age slower

[Srayman]

Seems good to me.