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

Add SignerEvent::NewNakamotoBlock and do not update a block to GloballyAccepted until event arrives #5516

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

jferrant
Copy link
Collaborator

@jferrant jferrant commented Nov 27, 2024

Closes: #5514

@jferrant jferrant changed the title Add SignerEvent::NewNakamotoBlock and do not update a block to GloballyAccepted until it this event arrives WIP: Add SignerEvent::NewNakamotoBlock and do not update a block to GloballyAccepted until it this event arrives Nov 28, 2024
@jferrant jferrant changed the title WIP: Add SignerEvent::NewNakamotoBlock and do not update a block to GloballyAccepted until it this event arrives WIP: Add SignerEvent::NewNakamotoBlock and do not update a block to GloballyAccepted until event arrives Nov 28, 2024
@jferrant jferrant force-pushed the feat/signer-subscribe-to-block-events branch from 7fefa19 to c107ca3 Compare December 2, 2024 18:11
…lyAccepted until node processes the new block successfully

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant force-pushed the feat/signer-subscribe-to-block-events branch from c107ca3 to 1855264 Compare December 2, 2024 18:26
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant force-pushed the feat/signer-subscribe-to-block-events branch from bf41d77 to 57848b3 Compare December 2, 2024 22:17
… into feat/signer-subscribe-to-block-events
@jferrant jferrant changed the title WIP: Add SignerEvent::NewNakamotoBlock and do not update a block to GloballyAccepted until event arrives Add SignerEvent::NewNakamotoBlock and do not update a block to GloballyAccepted until event arrives Dec 2, 2024
@jferrant jferrant marked this pull request as ready for review December 2, 2024 23:59
@jferrant jferrant requested review from a team as code owners December 2, 2024 23:59
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant force-pushed the feat/signer-subscribe-to-block-events branch 3 times, most recently from a25bd75 to f2da5f2 Compare December 3, 2024 00:35
Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

I think an integration test would be needed, otherwise LGTM!

Is /new_block always sent to an observer, regardless of the event keys they've set? It seems so, but I'd want to be 100% on that

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant force-pushed the feat/signer-subscribe-to-block-events branch from f2da5f2 to d4860f8 Compare December 3, 2024 00:37
@jferrant
Copy link
Collaborator Author

jferrant commented Dec 3, 2024

I think an integration test would be needed, otherwise LGTM!

Is /new_block always sent to an observer, regardless of the event keys they've set? It seems so, but I'd want to be 100% on that

It is always yes. But I cannot for the life of me test this without so much jankiness that I think it defeats the purpose. We already have multiple tests that pass through this exact code so I think its fine to not have an integration test (essentially order of ops is:

  1. Block N Locally rejected for reasons by a signer A (I forced this through testing directive)
  2. Signer A see 70% acceptance from OTHER signers. Mark is locally accepted. (Can force signers to ignore these signatures if we want to avoid setting the state to locally accepted)
  3. Get new block event: mark globally accepted. (Can force signers to either ignore this or force node to never announce the new block)

If 2 and 3 happen out of order, it doesn't make operational difference. We don't care if we are first globally and then see 70% or if we are first locally accepted and then mark globally once new block arrives.
If I force the signer A to ignore the 70% signatures and prevent the node from broadcasting a new burn block, we have to ALSO prevent the miner from appending it to the chain or susbsequent proposals at the same height N' will still fail the block validate Ok response. I can just verify that a signer passes the first check and we see a block validate submttal. Just not sure how useful this test actually is especially with all the janky forcing of situations.

EDIT: basically I cannot force a unique situation that PROVES that global acceptance happened ONLY at the new burn block arrival vs the reading of the nodes chain tip. I essentially need access to the signerdb and see the order of its updates is as expected and even then its not exactly unique. We have two instances where a block gets marked globally accepted and its very hard to force a scenario that does not cause the other to hit first/prove which one actually hit. Closest to is to forcibly propose a sister block with all other overrides disabled.

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant requested a review from a team as a code owner December 3, 2024 01:28
hstove
hstove previously approved these changes Dec 3, 2024
Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

LGTM!

@aldur aldur requested review from kantai and obycode December 3, 2024 15:54
@jferrant jferrant requested review from obycode and hstove December 6, 2024 16:15
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant requested a review from jcnelson December 9, 2024 14:33
obycode
obycode previously approved these changes Dec 9, 2024
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

LGTM!

… into feat/signer-subscribe-to-block-events
@jferrant jferrant requested a review from a team as a code owner December 10, 2024 14:12
@jferrant jferrant requested a review from obycode December 10, 2024 14:13
@aldur aldur added this to the 3.1.0.0.2 milestone Dec 11, 2024
)));
}
#[derive(Debug, Deserialize)]
struct TempBlockEvent {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of making a bespoke and scoped struct, it might be cleaner to just add a fn from_slice(bytes: &[u8]) -> Result<SignerEvent<T>, EventError> function to SignerEvent<T>. Then, you could put all of the decode logic into that function, and just pass in body.as_bytes().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think I fully understand what you mean by passing in body.as_bytes(). But I did make a more generic process_event function that will correctly deserialize the request bytes so long as the event implements serde and try_from for SignerEvent.

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you for addressing this! Just a few minor comments before approving.

I like what you've done to clean up the test flags. Do you think you could take a moment and add a couple sentences on what each one does (or open an issue for doing so if it's too out-of-scope), so future test authors know how to use them? Thanks!

@jferrant
Copy link
Collaborator Author

This looks great. Thank you for addressing this! Just a few minor comments before approving.

I like what you've done to clean up the test flags. Do you think you could take a moment and add a couple sentences on what each one does (or open an issue for doing so if it's too out-of-scope), so future test authors know how to use them? Thanks!

Good idea! Will do now!

… into feat/signer-subscribe-to-block-events
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
… into feat/signer-subscribe-to-block-events
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant requested a review from jcnelson December 12, 2024 16:17
… into feat/signer-subscribe-to-block-events
@jferrant jferrant force-pushed the feat/signer-subscribe-to-block-events branch from 061a93e to 2a4371f Compare December 13, 2024 20:27
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
obycode
obycode previously approved these changes Dec 13, 2024
… into feat/signer-subscribe-to-block-events
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant requested a review from obycode December 16, 2024 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: In Review
Development

Successfully merging this pull request may close these issues.

None yet

5 participants