prototype: compact blocks #1191
Conversation
cmwaters
changed the title
mempool/cat/block_builder.go
Outdated
| has := memR.mempool.store.has(key) | ||
| if !has { | ||
| // If the mempool provided the initial transactions yet received from | ||
| // consensus a transaction it doesn't recognize, this implies that | ||
| // either a tx was mutated or was added by the application. In either | ||
| // case, it is likely no other mempool has this transaction so we | ||
| // preemptively broadcast it to all other peers | ||
| // | ||
| // We don't set the priority, gasWanted or sender fields because we | ||
| // don't know them. | ||
| wtx := newWrappedTx(tx, key, memR.mempool.Height(), 0, 0, "") | ||
| memR.broadcastNewTx(wtx) | ||
| // For safety we also store this transaction in the mempool (ignoring | ||
| // all size limits) so that we can retrieve it later if needed. Note | ||
| // as we're broadcasting it to all peers, we should not receive a `WantTx` | ||
| // unless it gets rejected by the application in CheckTx. | ||
| // | ||
| // Consensus will have an in memory copy of the entire block which includes | ||
| // this transaction so it should not need it. | ||
| memR.mempool.store.set(wtx) | ||
| } |
There was a problem hiding this comment.
do we need to call checktx to avoid accidently including invalid txs?
mempool/cat/block_builder.go
Outdated
| timeTaken := request.TimeTaken() | ||
| if timeTaken == 0 { | ||
| return | ||
| } |
There was a problem hiding this comment.
It's for gathering metrics
| go func() { | ||
| reactor.ReceiveEnvelope(p2p.Envelope{ | ||
| Src: peer, | ||
| Message: &memproto.Txs{Txs: txs}, | ||
| ChannelID: mempool.MempoolChannel, | ||
| }) | ||
| }() |
There was a problem hiding this comment.
are we intenting to send all the txs to ourselves? I'm assuming no, since we are explicitly delaying the txs earlier and we can delete this and the test still passes.
| resultTxs, err := reactor.FetchTxsFromKeys(ctx, blockID, keys) | ||
| require.NoError(t, err) | ||
| require.Equal(t, len(txs), len(resultTxs)) | ||
| for idx, tx := range resultTxs { | ||
| require.Equal(t, txs[idx], tx) | ||
| } | ||
| repeatResult, err := reactor.FetchTxsFromKeys(ctx, blockID, keys) | ||
| require.NoError(t, err) | ||
| require.Equal(t, resultTxs, repeatResult) | ||
| wg.Wait() |
There was a problem hiding this comment.
what does repeating and comparing test? can we document this if its something not obvious or benign?
There was a problem hiding this comment.
I think it might be to hit the cache
| return nil | ||
| } | ||
|
|
||
| func (cs *State) addCompactBlock(msg *CompactBlockMessage, peerID p2p.ID) error { |
There was a problem hiding this comment.
don't need the peerID here, I guess we don't have that linter setup here
| func (cs *State) addCompactBlock(msg *CompactBlockMessage, peerID p2p.ID) error { | |
| func (cs *State) addCompactBlock(msg *CompactBlockMessage) error { |
| ) (*types.Block, *types.PartSet) { | ||
| ) *types.Block { |
There was a problem hiding this comment.
why get rid of the partset here?
consensus/state.go
Outdated
| cs.sendInternalMessage(msgInfo{&ProposalMessage{proposal}, ""}) | ||
| cs.sendInternalMessage(msgInfo{&CompactBlockMessage{ | ||
| Block: block, | ||
| Round: p.Round, | ||
| }, ""}) |
There was a problem hiding this comment.
after prototyping, just a little note here to just include the hashes in the canonical proposal to ensure that the hashes are not tampered
There was a problem hiding this comment.
niche meme, but worth a shot
There was a problem hiding this comment.
after prototyping, just a little note here to just include the hashes in the canonical proposal to ensure that the hashes are not tampered
What do you mean by this?
There was a problem hiding this comment.
is the compact block signed over by the proposer?
There was a problem hiding this comment.
No, the user tries to construct the compact block and matches that to the proposal block ID which was signed over by the proposer
| txs := make([][]byte, numTxs) | ||
| keys := make([][]byte, numTxs) | ||
| peer := genPeer() | ||
| blockID := tmhash.Sum([]byte("blockID")) |
There was a problem hiding this comment.
is the block id used to check anything or does it just serve as an index?
| FetchKeysFromTxs(context.Context, [][]byte) ([][]byte, error) | ||
| // For reconstructing the full block from the compact block | ||
| FetchTxsFromKeys(context.Context, []byte, [][]byte) ([][]byte, error) |
There was a problem hiding this comment.
note for after prototype: we need var names plus more dovs here
| mp.EnableTxsAvailable() | ||
| } | ||
| reactor.SetLogger(logger) | ||
|
|
There was a problem hiding this comment.
it won't let me highlight the part I want, the default will return nil (if not selecting v2 in the config), which could be confusing. The consensus reactor will pick a nil txFetcher, which uses will just return the keys instead of actually getting the txs.
There was a problem hiding this comment.
It assumes that compact blocks is not being used in which case the keys are the full transactions
No description provided.