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

feat!: new notify usage #195

Closed
wants to merge 4 commits into from
Closed

feat!: new notify usage #195

wants to merge 4 commits into from

Conversation

SWvheerden
Copy link
Contributor

Description

Changes how new tip notify works.
Blocks will only propagate new_tip_notify if it accepts the new tip
Sent out new tip notify if you did a catchup sync and got a new tip

},
};
let notify = NotifyNewTipBlock::new(local_peer_id, tip_blocks);
let _unused = notify_channel.send(notify);
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels wrong here. We will publish many newtips, instead of only the miner who sends it at the moment

@@ -652,6 +637,24 @@ impl ShareChain for InMemoryShareChain {
.build()?)
}

async fn get_tip_and_uncle_blocks(&self) -> Result<Vec<Arc<P2Block>>, ShareChainError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This was previously unused. Is it used now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah

@@ -66,8 +66,8 @@ impl P2ChainLevel {
Ok(())
}

pub fn block_in_main_chain(&self) -> Option<&Arc<P2Block>> {
self.blocks.get(&self.chain_block)
pub fn block_in_main_chain(&self) -> Option<Arc<P2Block>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I like this. Perhaps get_block_in_main_chain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

@SWvheerden
Copy link
Contributor Author

replaced by #224

@SWvheerden SWvheerden closed this Dec 11, 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.

2 participants