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

Publish/validate before db commit #2856

Merged
merged 8 commits into from
Oct 8, 2024
Merged

Conversation

paberr
Copy link
Member

@paberr paberr commented Aug 26, 2024

This PR adds a post-validation hook to the blockchain and uses it to publish/validate blocks before the database commit is happening. This way, the blocks are forwarded to the network without waiting for the database.

Pull request checklist

  • All tests pass. The project builds and runs.
  • I have resolved any merge conflicts.
  • I have resolved all clippy and rustfmt warnings.

@paberr paberr self-assigned this Aug 26, 2024
Copy link
Member

@nibhar nibhar left a comment

Choose a reason for hiding this comment

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

Lots of additional Clone derives, which indicate that we now clone more. Not entirely sure where this is and how big the impact is, pretty sure it could be avoided though from what I read.

blockchain/src/blockchain/mod.rs Outdated Show resolved Hide resolved
blockchain/src/blockchain/push.rs Show resolved Hide resolved
blockchain/src/blockchain/mod.rs Outdated Show resolved Hide resolved
blockchain/src/blockchain/mod.rs Outdated Show resolved Hide resolved
@nibhar
Copy link
Member

nibhar commented Aug 27, 2024

In bad136e, which is on top of this PR, I removed a bunch of the clones again, and passed the arguments to post_validation by reference.

@nibhar
Copy link
Member

nibhar commented Aug 28, 2024

In https://github.com/nimiq/core-rs-albatross/compare/nibhar/publish-pre-commit?expand=1 I also added the changed documentation, as well as a push_with_hook function which can be used to include the hook, whereas push remains unchanged. That cuts down on all those &() additions.

Copy link
Member

@Eligioo Eligioo left a comment

Choose a reason for hiding this comment

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

LGTM, nice way to decouple the publish and commit. 1 small log thing.

blockchain/src/blockchain/push.rs Show resolved Hide resolved
validator/src/validator.rs Outdated Show resolved Hide resolved
blockchain/src/blockchain/push.rs Outdated Show resolved Hide resolved
blockchain/src/blockchain/push.rs Outdated Show resolved Hide resolved
blockchain/src/blockchain/push.rs Outdated Show resolved Hide resolved
Comment on lines 22 to 25
pub(crate) enum ProduceMicroBlockEvent {
MicroBlock(MicroBlock, PushResult),
MicroBlock,
}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this event is now unused and we could get rid of it. But it's also not breaking anything so I think it would be fine to address this in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yes it is unused and could be removed.

Removing it will likely also entail making the micro Producer no longer having to implement Stream. That probably will mean a bunch of additional changes (for the better).

I opted not to do it here as it is out of scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the "fix clippy warning" commit could be dropped instead of half-removing the MicroBlock event. A #[allow()] could be used instead, leaving the (full) removal for a future PR.

Copy link
Member

Choose a reason for hiding this comment

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

For the time being I don't plan on doing any additional work there. The Event is unused, but removing the stream makes us less flexible in the future. If the micro producer is ever going to return something meaningful again it would have to be re implemented. It does not break anything as it is and I think it is semantically not too bad either.

@nibhar nibhar force-pushed the pb/publish-pre-commit branch 2 times, most recently from e7dc06c to b113d42 Compare September 27, 2024 12:39
Copy link
Member

@styppo styppo left a comment

Choose a reason for hiding this comment

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

LGTM

@jsdanielh
Copy link
Member

Rebasing branch and signing commits for merge

@jsdanielh jsdanielh merged commit 93e9788 into albatross Oct 8, 2024
7 checks passed
@jsdanielh jsdanielh deleted the pb/publish-pre-commit branch October 8, 2024 19:48
@jsdanielh jsdanielh added this to the Nimiq PoS Mainnet milestone Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants