Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix slot missing transaction #923

Merged

Conversation

Neylix
Copy link
Member

@Neylix Neylix commented Feb 28, 2023

Description

Actually, when it's the time for a new slot, the slot timer send a event to all the subset to process the slot creation.
But due to some latency for a slot node to receive the ReplicationAttestation message or simply the latency for a transaction to be replicated, the slot node may have build and sent the slot before receiving all the slot transaction.

For example:
image

To solve this problem, here is the new workflow:

  • We don't verify that a replication attestation is in the right slot time. Any attestation can be in any slot.
  • When building the summary, we control that the number of confirmations of an attestation reach a certain threshold (35%) of the expected confirmations number. If it not reaches the threshold, the attestation is postponed to the next summary.
  • As we can have postponed attestation in a next summary, we ensure the attestation is between 2 previous summary and current summary.

Here is an example:

image

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@Neylix Neylix added bug Something isn't working beacon chain Involve BeaconChain labels Feb 28, 2023
@Neylix Neylix marked this pull request as draft February 28, 2023 15:51
@Neylix Neylix force-pushed the Fix-slot-missing-transaction branch 2 times, most recently from d9cb14e to cffa0bd Compare February 28, 2023 18:17
@Neylix Neylix marked this pull request as ready for review February 28, 2023 18:18
@Neylix Neylix marked this pull request as draft March 1, 2023 18:04
@Neylix
Copy link
Member Author

Neylix commented Mar 1, 2023

After some discussion, a better way to handle this problem has been found:
When a node receive a replication attestation it will add it to the current slot if the tx timestamp is over than 2 previous summary time even if the slot time is not the right one.
When it is slot time, every node will send the slot if it is not empty even if the node is not an elected one for the current slot.
On summary time, before adding a replication attestation, the node ensure that there is enough confirmations with a threshold of 35% minimum (threshold if there is at least 10 expected replication nodes) If the threshold is not reached, the attestation is postponed to the next summary by just adding the attestation in the current slot.
During self repair, the same threshold control is performed

@Neylix Neylix force-pushed the Fix-slot-missing-transaction branch from cffa0bd to b87aeac Compare March 3, 2023 13:33
@Neylix Neylix marked this pull request as ready for review March 3, 2023 13:45
@Neylix Neylix force-pushed the Fix-slot-missing-transaction branch 2 times, most recently from bcd76fc to e2631f1 Compare March 6, 2023 11:43
@bchamagne
Copy link
Member

bchamagne commented Mar 6, 2023

As discussed, there is an issue with the transactions from last slot being duplicated in 2 summaries (S and S+1).

@Neylix
Copy link
Member Author

Neylix commented Mar 6, 2023

As discussed, there is an issue with the transaction from last slot being duplicated in 2 summaries (S and S+1).

I found a way to fix it in this PR, I'll move the threashold verification when creating the aggregate

@Neylix Neylix force-pushed the Fix-slot-missing-transaction branch from e2631f1 to 0f210f4 Compare March 6, 2023 18:16
@Neylix Neylix requested a review from bchamagne March 6, 2023 18:17
Copy link
Member

@bchamagne bchamagne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with 3 nodes (minimum_nodes_for_threshold = 2)
I added a 10s sleep to 2/3 nodes.
I created a transfer few second before summary
Transfer postponed as expected. ✔️

Screenshot 2023-03-07 at 11-46-16 ARCHETHIC

(we see the transfer at 40:54 present in the 41->42 summary as expected)

@Neylix Neylix force-pushed the Fix-slot-missing-transaction branch from 9467391 to 3b842b9 Compare March 13, 2023 10:11
if it is not a beacon summary node
to have all confirmations available
instead of rejecting entire summary if one attestation is invalid
if refused, attestation is postponed to next summary
@Neylix Neylix force-pushed the Fix-slot-missing-transaction branch from 3b842b9 to a36a491 Compare March 13, 2023 11:54
@Neylix Neylix merged commit 43b8dce into archethic-foundation:develop Mar 15, 2023
@Neylix Neylix deleted the Fix-slot-missing-transaction branch March 15, 2023 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beacon chain Involve BeaconChain bug Something isn't working
Projects
Status: Done 🍻
Development

Successfully merging this pull request may close these issues.

None yet

3 participants