Skip to content

Conversation

@qizhou
Copy link
Contributor

@qizhou qizhou commented Apr 15, 2019

Implement two-phase commitment of adding minor blocks

  • Add AddMinorBlockHeaderListRequest/Response so that the blocks downloaded from master (root block sync) will be also sent to master from slaves explicitly - so that a slave knows whether the blocks are committed or not
  • Both new minor block and blocks downloaded by root block sync will use similar logic to determine whether a block is committed or not, and may redo if it is not committed
  • This fixes an issue that a cross-shard transaction is broadcasted while the cluster is shutdown. Original logic won't broadcast the cross-shard tx again (since the slaves finds the block in local db)

@qizhou qizhou requested a review from a team April 15, 2019 20:31
Copy link
Contributor

@ninjaahhh ninjaahhh left a comment

Choose a reason for hiding this comment

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

mind adding some tests? I know it could be kinda hard to simulate unexpected shutdown...

coinbase_addrs = self.__get_coinbase_addresses_until_block(header_hash, length)
return Counter(coinbase_addrs)

def is_committed_by_hash(self, h):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: prefer letting the caller directly access shard_state.db and call is_minor_block_committed_by_hash / commit_minor_block_by_hash directly. didn't see many benefits over additional wrapping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As an abstraction, the client of state_shard should not access db directly - i.e., we should probably rename db to __db to enforce that.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. however currently shard_state.db is directly accessed in many other places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is bad, and we should fix that....

@qizhou qizhou force-pushed the 2phase_minor_block_commit branch from 101413c to 58f6ee5 Compare April 16, 2019 18:14
coinbase_addrs = self.__get_coinbase_addresses_until_block(header_hash, length)
return Counter(coinbase_addrs)

def is_committed_by_hash(self, h):
Copy link
Contributor

Choose a reason for hiding this comment

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

ok. however currently shard_state.db is directly accessed in many other places

@qizhou qizhou merged commit cf74767 into master Apr 17, 2019
@ninjaahhh ninjaahhh deleted the 2phase_minor_block_commit branch April 18, 2019 09:50
# Let's make sure all the shards and master got it before committing it
future = self.add_block_futures.get(block_hash, None)
if future is not None:
return BlockCommitStatus.COMMITTING, future
Copy link
Contributor

Choose a reason for hiding this comment

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

oopsy, can you spot the bug here? @qizhou

Belgarion pushed a commit to Belgarion/pyquarkchain_cuda that referenced this pull request Sep 8, 2020
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.

3 participants