[Consensus] Add MinBlockTime to delay mempool reaping

[red-0ne]

Description

This PR implements delaying mempool reaping by MinBlockTime, resetting the timer at every new height

All the logic lives in consensus/pacemaker/module.go, and the actual delay is happening consensus/hotstuff_leader.go

The mechanism ensures:

• Block proposal is made only once per block
• If the timer expires before having a block candidate, the first call to this method will trigger the block proposal
• If a late message is received after a block is proposed by another call, the late message is discarded
• Decoupling timer registration and signaling

Concurrent requests

sequenceDiagram
  participant ReapMempool
  participant BlockPrepare
  participant Delay

  ReapMempool->>BlockPrepare:RequestPrepare (id=1)
  BlockPrepare->>+Delay:Start delay
  Note right of Delay: Wait for minBlockTime
  ReapMempool->>BlockPrepare:RequestPrepare (id=2)
  BlockPrepare--xReapMempool:doNotPrepare (id=1)
  Delay->>-BlockPrepare:End delay
  BlockPrepare->>ReapMempool:doPrepare (id=2)
  ReapMempool->>BlockPrepare: Prepare block

Loading

Late requests

sequenceDiagram
  participant ReapMempool
  participant BlockPrepare
  participant Delay

  BlockPrepare->>+Delay:Start delay
  Note right of Delay: Wait for minBlockTime
  Delay->>-BlockPrepare:End delay
  ReapMempool->>BlockPrepare:doPrepare (id=1)
  BlockPrepare->>ReapMempool:RequestPrepare (id=1)
  ReapMempool->>BlockPrepare: Prepare block

Loading

Summary generated by Reviewpad on 02 Aug 23 18:00 UTC

This pull request introduces several changes across multiple files:

1. The cli-client.yaml file includes the addition of a new environment variable, POCKET_IBC_HOST_PRIVATE_KEY, and changes to existing environment variables related to Postgres.
2. The block.go file has changes related to package imports and modifications to various functions.
3. The uow_leader.go file includes changes to import statements and modifications in the CreateProposalBlock and reapMempool functions.
4. The ibc/module.go file has a small modification relating to a function call change.
5. The ibc_handle_event_test.go file has a change related to the substitution of a method call.
6. The db.go file has changes related to comments, function additions, and modifications.
7. The utility/unit_of_work/block.go file includes changes to function implementations and comments.
8. The ibc_handler.go file introduces a new interface and associated methods.
9. The transaction.go file has changes related to the HandleTransaction function and import statements.
10. The pacemaker_test.go file introduces new unit tests.
11. The Tiltfile file includes changes relating to configurations and values.
12. The consensus_config.proto file has an addition of a new field.
13. The persistence_module.go file has changes relating to method modifications, interface additions, and a TODO implementation.
14. The runtime/manager.go file includes a comment regarding technical debt.
15. The manager.go file has import statement additions and a new function.
16. The bsc.go file has changes related to function renaming and updates to function calls.
17. The README.md file has information regarding the disabling of pre-commit changelog verification.

Please review these changes in detail to ensure they meet the requirements of the codebase.

Issue

Fixes #705

Type of change

Please mark the relevant option(s):

• New feature, functionality or library
• Bug fix
• Code health or cleanup
• Major breaking change
• Documentation
• Other

List of changes

• Change prop: the agreed upon repository structure #1
• Change Version Number #2
• ...

Testing

• make develop_test; if any code changes were made
• make test_e2e on k8s LocalNet; if any code changes were made
• e2e-devnet-test passes tests on DevNet; if any code was changed
• Docker Compose LocalNet; if any major functionality was changed or introduced
• k8s LocalNet; if any infrastructure or configuration changes were made

Required Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
• I have tested my changes using the available tooling
• I have updated the corresponding CHANGELOG

If Applicable Checklist

• I have updated the corresponding README(s); local and/or global
• I have added tests that prove my fix is effective or that my feature works
• I have added, or updated, mermaid.js diagrams in the corresponding README(s)
• I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

[reviewpad]

Thank you @red-0ne for this first contribution!

[Olshansk]

@red-0ne Please make sure to update the issues & PR meta fields in the future. Updated it in this one for reference.

[Olshansk]

I did not review consensus/e2e_tests/pacemaker_test.go yet.

Just minor comments / nits.

Interesting approach but I do want to look at it with fresh eyes tomorrow. Just want to see if we can make it a bit simpler.

Other than that, it's a fucking awesome start!

[red-0ne]

Just want to see if we can make it a bit simpler.

We can. True that this implementation is intricate. It's coupling timer registration with the client/listener logic which should not. I will split this into a timer registration function and a listening one. It would be much simpler to reason about.

[red-0ne]

@Olshansk the logic should be more readable and ready to review now. Which makes me think about the nature of the delay.

We are currently enforcing it at the leader level. Which means that if the leader wants to prepare the block earlier, there's nothing enforcing it not to do so.

But if we place the delay at the new round message emission point. All (honest) replicas will delay their new round messages so that the leader could not reach the consensus and build the block earlier. This would mean that each block proposal time is enforced by the whole network instead of by its leader only. We could floor the delay imposed by replicas (eg. -10%) and let the leader that window to fine grain the timer.

The problem I see with it, is that it makes reaching consensus always a hurry task, an adjustable offset could do the trick and even be dynamic. Also choreographing the replicas to distribute their deadlines over MinBlockTime period may also be worth digging.

This should not be that much of a change 😄 Since at the new round construction level, any node knows if it's the leader of that round or not:

• If it is a leader
  • Registers the timer at that exact moment (the earliest a node knows there's a new round and that it's the leader)
  • Sends the new round to itself (acts like a replica)
  • If it reaches new round quorum it prepares the block by applying the delay logic according to its local timer
• If it's a replica
  • Registers the timer at that exact moment (the earliest that replica commits for the new round --should be same as leader)
  • Delays broadcasting new round message to the leader by MinBockTime - AdjustableOffset ± (Randomness or distribution function)

--
This way, for a 15minutes block time the whole network could eg. enforce that a block is proposed no earlier than 14minutes, and leader makes sure it lands at the 15minutes mark

This may be overprotocoling too 🤣

[Olshansk]

@red-0ne

1. Can you create a new GitHub Issue for your comment. The idea has legs and it's important to discuss but outside the scope of this ticket.

2. You have great diagrams in the github description, but it'll get lost and forgotten. Can you add it to a README either in this PR or another one?

[red-0ne]

You have great diagrams in the github description, but it'll get lost and forgotten. Can you add it to a README either in this PR or another one?

I will create pace maker documentation along the requested changes.

[Olshansk]

I will create pace maker documentation along the requested changes.

Sounds great!

• Please re-request the review when I should take another look
• For comments that are tended to and don't need an action, go ahead and resolve them. If more conversation is needed, leave them open and we will keep the discussion going.

Note to self: Add the above to the code review guidelines.

[red-0ne]

I will create pace maker documentation along the requested changes.

PaceMaker documentation added to PR#915

[red-0ne]

@Olshansk , I resolved most of the change requests. Only remains these two discussions:

• Exposing ProcessDelayedBlockPrepare on /shared which I don't think we should do.
• Deferring mutex.Unlock

Anything else to re-check before finalizing it?

[red-0ne]

Also, I wanted to confirm that you tested this with Localnet and everything worked fine end-to-end? +1 ?

The delay is effectively applied when running in non-manual mode. Even if I had issues with block building (which seems to be coming from somewhere else).

{"level":"info","module":"consensus","kind":"LEADER","height":0,"round":68,"step":1,"time":"2023-08-02T17:48:42Z","message":"📬 Received enough 📬 votes"}
{"level":"info","module":"consensus","kind":"LEADER","time":"2023-08-02T17:48:42Z","message":"Preparing a new block - no prepareQC found"}
{"level":"debug","module":"leader_utility_UOW","maxTxBytes":500000000,"proposer":"00104055c00bed7c983a48aac7dc6335d7c607a7","source":"CreateProposalBlock","time":"2023-08-02T17:48:47Z","message":"calling beginBlock"}

[Olshansk]

@red-0ne

The delay is effectively applied when running in non-manual mode.

That makes sense. This is intended for non-manual mode only. Wdyt of adding an assertion for this?

Even if I had issues with block building (which seems to be coming from somewhere else).

Consider trying to revert to the main branch and seeing if you see the same issues in that context. If that's the case, and it's an issue on main, I'll jump in to debug immediately. If not, let me know if/when I should dive deeper into the code in this PR.

[red-0ne]

@Olshansk

That makes sense. This is intended for non-manual mode only. Wdyt of adding an assertion for this?

You mean enable the feature only when manual is false ?

Consider trying to revert to the main branch and seeing if you see the same issues in that context. If that's the case, and it's an issue on main, I'll jump in to debug immediately. If not, let me know if/when I should dive deeper into the code in this PR.

Created an issue for that #965, it is not blocking this work.

[Olshansk]

You mean enable the feature only when manual is false ?

Yes.

If manual == true => skip code path for minBlockTime

[Olshansk]

This PR has 106 lines changed relative to main. Please merge with main and resolve conflicts and re-request a review then.