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

Handle transactions towards metachain in DbLookupExtensions #2256

Merged
merged 10 commits into from
Aug 30, 2020

Conversation

andreibancioiu
Copy link
Collaborator

@andreibancioiu andreibancioiu commented Aug 29, 2020

The metachain directly notarizes miniblocks towards it - at commit block time. Therefore, there's the time to set the hyperblock coordinates for the miniblocks in question. At the source shard, the "block notarized" notifications arrive in a slightly different manner - the payload is a block.Header, not a block.MetaBlock.

Therefore,

  • Gather hyperblock coordinates for transactions towards meta as well
  • Refactoring: separate normal from invalid transactions in nodeTransactions.go
  • Return extra fields for unsigned transactions (e.g. return message, original transaction hash)
  • Improve transaction status (API) reporting when DatabaseLookupExtensions are enabled (that is, return executed if hyperblock coordinates are fully set, that is, if parent miniblock is fully notarized)
  • Fix value of transaction type (API) when DatabaseLookupExtensions, and miniblock metadata is present.

How to test:

  • Send a staking transaction, then inspect its hyperblock coordinates
  • Send invalid transaction, check its Type field
  • Also inspect hyperblock coordinates for some rewards transactions (as a regression test)

@andreibancioiu andreibancioiu added the type:feature New feature or request label Aug 29, 2020
@sasurobert sasurobert self-requested a review August 29, 2020 17:20
sasurobert
sasurobert previously approved these changes Aug 29, 2020
TransactionData []byte
SelfShard uint32
MiniblockType block.Type
IsMiniblockFullyNotarized bool
Copy link
Contributor

Choose a reason for hiding this comment

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

IsMiniblockFinalized ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sasurobert Isn't the block finalized only at n+1 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

IsMiniBlockFully notarized is for when even the destination processes the miniblock and the result of the process is finalized, included on meta, and meta is finalized.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

LucianMincu
LucianMincu previously approved these changes Aug 29, 2020
Copy link
Contributor

@LucianMincu LucianMincu left a comment

Choose a reason for hiding this comment

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

System tests passed.

@@ -228,27 +239,45 @@ func (hr *historyRepository) onNotarizedBlocks(shardID uint32, headers []data.He
for i, header := range headers {
headerHash := headersHashes[i]

metaBlock, ok := header.(*block.MetaBlock)
if !ok {
asMetaBlock, isMetaBlock := header.(*block.MetaBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

metaBlock, isMetaBlock

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed.

return
}

for _, shardData := range metaBlock.ShardInfo {
hr.onNotarizedBlock(metaBlock.GetNonce(), headerHash, shardData)
asBlock, isBlock := header.(*block.Header)
Copy link
Contributor

Choose a reason for hiding this comment

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

header, isHeader

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed.

hr.onNotarizedBlock(metaBlock.GetNonce(), headerHash, shardData)
asBlock, isBlock := header.(*block.Header)
if isBlock {
if asBlock.GetShardID() != core.MetachainShardId {
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition is always true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed then.

if isMetaBlock {
for _, shardData := range asMetaBlock.ShardInfo {
hr.onNotarizedInMetaBlock(asMetaBlock.GetNonce(), headerHash, &shardData)
}
return
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to iterate the whole list ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@SebastianMarian, not sure what you mean - we iterate on the whole list of ShardInfos. What is not needed, it's discarded in onNotarizedMiniblock, of course - see logic around iDontCare.

Copy link
Contributor

Choose a reason for hiding this comment

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

I referred to the whole list of headers, as you call return after the iteration of all the shardData from first headerHandler. It is ok ? I mean that this return should not be replaced with continue ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, now I see. Will fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

continue
}

hr.onNotarizedInRegularBlockOfMeta(asMetaBlock.GetNonce(), headerHash, asBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

asBlock instead asMetaBlock? asMetaBlock will be nil and the code will panic here if asBlock is not nil and isBlock is true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch 🥳

My unit test was hiding that - see what I've changed there 🤯

(wasn't panicking, it returned 0 - also hidden in our system tests, because we were looking at destination for hyperblock coordinates, at source they would have been bad, nonce = 0)

}
}

func (hr *historyRepository) onNotarizedBlock(metaBlockNonce uint64, metaBlockHash []byte, blockHeader block.ShardData) {
func (hr *historyRepository) onNotarizedInMetaBlock(metaBlockNonce uint64, metaBlockHash []byte, blockHeader *block.ShardData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

shardData instead blockHeader

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed.

sasurobert
sasurobert previously approved these changes Aug 30, 2020
LucianMincu
LucianMincu previously approved these changes Aug 30, 2020
Copy link
Contributor

@LucianMincu LucianMincu left a comment

Choose a reason for hiding this comment

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

System tests passed.

iulianpascalau
iulianpascalau previously approved these changes Aug 30, 2020
if tx, ok := txObj.(*transaction.Transaction); ok {
return n.prepareNormalTx(tx)
}
case transaction.TxTypeInvalid:
if tx, ok := txObj.(*transaction.Transaction); ok {
return n.prepareInvalidTx(tx)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@LucianMincu LucianMincu merged commit e49b34c into development Aug 30, 2020
@LucianMincu LucianMincu deleted the hyperblock-metachain-txs branch August 30, 2020 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants