Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Enacted & Retracted blocks' transactions removed/added to transaction… #674

Closed
wants to merge 5 commits into from

Conversation

tomusdrw
Copy link
Collaborator

… queue

Closes #649

@tomusdrw tomusdrw added the A0-pleasereview 🤓 Pull request needs code review. label Mar 11, 2016
@tomusdrw
Copy link
Collaborator Author

Not sure where (if) should I merge good+enacted bad+retracted.

@gavofyork
Copy link
Contributor

i'd say generally "good" should become "enacted" and "bad" to "retracted". i don't think they're used for different meanings.

@tomusdrw
Copy link
Collaborator Author

I agree that good \subseteq enacted, but bad are blocks that have never made to the chain - not sure if it's ok to call them retracted. So in general the question is what to pass in SyncMessage::NewChainBlocks - I'm good with passing just enacted and retracted and doing the merge in client.

Can you confirm my assumption that \forall x | x \in good \implies x \in enacted?

@@ -230,12 +230,36 @@ impl<V> Client<V> where V: Verifier {
Ok(closed_block)
}

fn calculate_enacted_retracted(&self, routes: Vec<ImportRoute>) -> (Vec<H256>, Vec<H256>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what is routes?

@gavofyork
Copy link
Contributor

ahh so:

  • "good" actually means "imported";
  • "bad" means "invalid";
  • "enacted" means was not previously in the canon chain, and now is;
  • "retracted" means was previously in the canon chain, and now is not.

as such, none of them are subsets of the other.

@gavofyork
Copy link
Contributor

i suggest we rename good and bad to imported and invalid, and place a comprehensive description somewhere very visible in the documentation.

@tomusdrw
Copy link
Collaborator Author

But isn't good - (enacted + retracted) = 0 (all blocks that are in good are actually included in one of enacted/retracted?

@@ -293,11 +321,12 @@ impl<V> Client<V> where V: Verifier {

{
if !good_blocks.is_empty() && self.block_queue.queue_info().is_empty() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gavofyork Is the block_queue is empty check needed here?
My understanding is that if we don't send the event - some of the blocks' transactions will not be removed from queue because of that (cause in next event the blocks won't be there)

@gavofyork
Copy link
Contributor

0 -> 1 -> 2a -> 3a -> 4a
       \-> 2b

3b, 4b, 5b, 3c

good: 3b, 4b, 5b, 3c
enacted: 2b, 3b, 4b, 5b
retracted: 4a, 3a, 2a

0 -> 1 -> 2a -> 3a -> 4a
       \-> 2b -> 3b -> 4b -> 5b
              \-> 3c

(all disjoint)

@gavofyork gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 12, 2016
@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Mar 12, 2016
@gavofyork
Copy link
Contributor

Fails on:

thread 'chain::tests::should_add_transactions_to_queue' panicked at 'assertion failed: (left == right) (left: 0, right: 1)', sync/src/chain.rs:1664

@gavofyork gavofyork added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 13, 2016
gavofyork added a commit that referenced this pull request Mar 13, 2016
@gavofyork
Copy link
Contributor

now contained in #700

@gavofyork gavofyork closed this Mar 13, 2016
@tomusdrw tomusdrw deleted the tx_queue_retracted branch March 15, 2016 08:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants