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: chainstore: implement TipSetBlockMessagesReceipts method #9186

Closed
wants to merge 2 commits into from

Conversation

frrist
Copy link
Member

@frrist frrist commented Aug 18, 2022

  • TipSetBlockMessagesReceipts returns the blocks and messages in a tipset and their corresponding
    receipts from its parent tipset matching block order in tipset.

Additional Info

Will be consumed by lily

Checklist

Before you mark the PR ready for review, please make sure that:

  • All commits have a clear commit message.
  • The PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
    • area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
  • This PR has tests for new functionality or change in behaviour
  • If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
  • CI is green

@frrist frrist requested a review from a team as a code owner August 18, 2022 19:09
@frrist frrist requested review from arajasek and removed request for arajasek August 18, 2022 21:03
@frrist frrist marked this pull request as draft August 19, 2022 01:57
@frrist frrist force-pushed the frrist/block-msg-receipt-method branch from 0baec3d to 4541329 Compare August 19, 2022 03:31
@frrist frrist marked this pull request as ready for review August 19, 2022 03:31
@frrist frrist requested a review from ZenGround0 August 22, 2022 19:23
Comment on lines +342 to +355
msgIdx := 0
// index or receipts in `out.Receipts`
receiptIdx := 0
Copy link
Member Author

Choose a reason for hiding this comment

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

use one counter

@@ -300,3 +301,144 @@ func (cs *ChainStore) LoadSignedMessagesFromCids(ctx context.Context, cids []cid

return msgs, nil
}

// TipSetBlockMessagesReceipts returns the blocks and messages in `ts` and their corresponding receipts from `pts` matching block order in tipset (`ts`).
Copy link
Member Author

Choose a reason for hiding this comment

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

this comment is backwards, will fix

Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

Went over offline, generally looks good. @magik6k you might want to take a look to make sure we want this and you like how it is

Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

nvm I need to look more closely

@frrist frrist requested a review from magik6k August 24, 2022 23:15
chain/store/messages.go Outdated Show resolved Hide resolved
chain/store/messages.go Outdated Show resolved Hide resolved
chain/store/messages.go Show resolved Hide resolved
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

This looks correct, but is fine code -- I still only have about 50% confidence. I don't think this is landable without a test.

// The Receipts are one-to-one with Messages index.
type BlockMessageReceipts struct {
Block *types.BlockHeader
// Messages contained in Block.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is misleading -- these are not the messages contained (included) in a block, but rather the subset of those messages that were executed in the context of pts.

receiptIdx := 0
out[blkIdx] = &BlockMessageReceipts{
// block containing messages
Block: pts.Blocks()[blkIdx],
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I would get pts.Blocks()[blkIdx] from the range operation.

// Receipts contained in Block.
Receipts []*types.MessageReceipt
// MessageExectionIndex contains a mapping of Messages to their execution order in the tipset they were included.
MessageExecutionIndex map[types.ChainMsg]int
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really the map you want? Can it be indexed by msg cid instead?

- TipSetBlockMessagesReceipts returns the blocks and messages in a tipset and their corresponding
receipts from its parent tipset matching block order in tipset.
- define ParentMessageReceipts method on tipset type
- spelling
- sanity checks in TipSetBlockMessagesReceipts
@frrist frrist force-pushed the frrist/block-msg-receipt-method branch from a07ee7e to d7951ee Compare September 20, 2022 23:06
@frrist
Copy link
Member Author

frrist commented Sep 20, 2022

I don't think this is landable without a test.

Roger. Is there any prior art here? I was hoping to find a test that exercises other methods in this file. Do you know of any?

@frrist
Copy link
Member Author

frrist commented Nov 17, 2022

The testing framework to validate this code change is not yet in place, will close and revisit it perhaps at a later time.

@frrist frrist closed this Nov 17, 2022
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.

None yet

3 participants