Signal irreversible_block after it is irreversible

[heifner]

Currently controller signals irreversible_block in log_irreversible before it is written to the block log and before forkdb root is advanced. This means any plugin that reaches back into the controller will not see the block as irreversible.

For example, take the net_plugin which connects to the irreversible_block signal and calls:

   void net_plugin_impl::update_chain_info() {
      controller& cc = chain_plug->chain();
      uint32_t lib_num = 0, head_num = 0;
      {
         fc::lock_guard g( chain_info_mtx );
         chain_info.lib_num = lib_num = cc.last_irreversible_block_num();
         chain_info.lib_id = cc.last_irreversible_block_id();
         chain_info.head_num = head_num = cc.fork_db_head_block_num();
         chain_info.head_id = cc.fork_db_head_block_id();
      }
      fc_dlog( logger, "updating chain info lib ${lib}, fork ${fork}", ("lib", lib_num)("fork", head_num) );
   }

When irreversible moves this above call of update_chain_info is useless as it never observes the new state.

debug 2023-12-14T20:54:06.941 nodeos    net_plugin.cpp:3946           on_irreversible_bloc ] on_irreversible_block, blk num = 309, id = 000001354f03fe5551150d8c0d8891ea2d50db42c5554c9bbb4177403048df1c
debug 2023-12-14T20:54:06.941 nodeos    net_plugin.cpp:3210           update_chain_info    ] updating chain info lib 70, fork 637
debug 2023-12-14T20:54:06.945 nodeos    net_plugin.cpp:3946           on_irreversible_bloc ] on_irreversible_block, blk num = 310, id = 0000013675a82dc0b646e35a5343b8bf064a627eec519ae3a6600486cda94baf
debug 2023-12-14T20:54:06.945 nodeos    net_plugin.cpp:3210           update_chain_info    ] updating chain info lib 70, fork 637

So even though LIB is 310, net_plugin still thinks it is 70 until another block is processed and update_chain_info is called outside of the irreversible_block signal.

I believe we need to modify controller::log_irreversible to signal all the blocks after the block log is updated the forkdb has been updated via forkdb.advance_root().

[bhazzard]

This isn't causing any known material issues currently. But because the behavior doesn't work as expected (and is objectively wrong), it could cause problems in the future due to confusion.

[bhazzard]

Because this would be a change to an outward facing interface (signaling), this should be done in a major release. Also this issue may be more relevant with Instant Finality because LIB is more noticeable with faster finality, therefore this would ideally be included in 6.0.0.

[bhazzard]

Per Kevin: This is one of those things we could just never fix. It causes confusion when implementing plugins as the signal comes before the state of the node is actually irreversible. However, it would be good to make this more "sane".

Would be nice to get it into 2.0 in my opinion.