Skip to content

Conversation

@miiu96
Copy link
Contributor

@miiu96 miiu96 commented Aug 27, 2024

Reasoning behind the pull request

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

  • was the PR targeted to the correct branch?
  • if this is a larger feature that probably needs more than one PR, is there a feat branch created?
  • if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

@miiu96 miiu96 self-assigned this Aug 27, 2024
@sstanculeanu sstanculeanu self-requested a review September 2, 2024 07:22
func (srcf *shardRequestersContainerFactory) generateReceiptRequesters() error {
// only one receipts topic
selfShardID := srcf.shardCoordinator.SelfId()
identifierReceipt := factory.ReceiptTopic
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
identifierReceipt := factory.ReceiptTopic
identifierReceipt := factory.ReceiptTopic + srcf.shardCoordinator.CommunicationIdentifier(selfShardID)

in order to make it intra shard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 121 to 156
unrequestedHashes := rrh.getUnrequestedHashes(receiptsHashes, uniqueReceiptSuffix)
if len(unrequestedHashes) == 0 {
return
}

log.Debug("requesting receipts from network",
"topic", factory.ReceiptTopic,
"num receipts", len(unrequestedHashes),
)

requester, err := rrh.requestersFinder.IntraShardRequester(factory.ReceiptTopic)
if err != nil {
log.Error("requestByHashes.CrossShardRequester",
"error", err.Error(),
"topic", factory.ReceiptTopic,
)
return
}

receiptRequester, ok := requester.(HashSliceRequester)
if !ok {
log.Warn("wrong assertion type when creating receipts requester")
return
}

for _, txHash := range receiptsHashes {
log.Trace("requestByHashes", "hash", txHash, "topic", factory.ReceiptTopic,
"num txs", len(unrequestedHashes),
"stack", string(debug.Stack()))
}

rrh.whiteList.Add(unrequestedHashes)

go rrh.requestHashesWithDataSplit(unrequestedHashes, receiptRequester)

rrh.addRequestedItems(unrequestedHashes, uniqueReceiptSuffix)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is more ore less duplicated with requestByHashes method.. why not simply reduce it to one line call:

rrh.requestByHashes(rrh.shardID, receiptsHashes, factory.ReceiptTopic, uniqueReceiptSuffix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored

ArgBaseRequester
}

type receiptRequester struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing unittests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added


var errFetch error
errorsFound := 0
mbsBuffSlice := make([][]byte, 0, len(hashes))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
mbsBuffSlice := make([][]byte, 0, len(hashes))
rcpBuffSlice := make([][]byte, 0, len(hashes))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


continue
}
mbsBuffSlice = append(mbsBuffSlice, mb)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
mbsBuffSlice = append(mbsBuffSlice, mb)
rcpBuffSlice = append(rcpBuffSlice, mb)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

mbsBuffSlice = append(mbsBuffSlice, mb)
}

buffsToSend, errPack := rcRes.dataPacker.PackDataInChunks(mbsBuffSlice, maxBuffToSendBulkMiniblocks)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
buffsToSend, errPack := rcRes.dataPacker.PackDataInChunks(mbsBuffSlice, maxBuffToSendBulkMiniblocks)
buffsToSend, errPack := rcRes.dataPacker.PackDataInChunks(rcpBuffSlice, maxBuffToSendBulkMiniblocks)

Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps add a new const instead of maxBuffToSendBulkMiniblocks, specific to receipt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

}

if errFetch != nil {
errFetch = fmt.Errorf("resolveMbRequestByHashArray last error %w from %d encountered errors", errFetch, errorsFound)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
errFetch = fmt.Errorf("resolveMbRequestByHashArray last error %w from %d encountered errors", errFetch, errorsFound)
errFetch = fmt.Errorf("resolveReceiptRequestByHashArray last error %w from %d encountered errors", errFetch, errorsFound)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

sstanculeanu
sstanculeanu previously approved these changes Sep 3, 2024
@raduchis raduchis requested review from raduchis and removed request for raduchis September 3, 2024 20:47
sstanculeanu
sstanculeanu previously approved these changes Sep 11, 2024
@miiu96 miiu96 merged commit 946d912 into feat/light-client Sep 11, 2024
@miiu96 miiu96 deleted the requester-resolver-receipt-data branch September 11, 2024 11:43
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.

4 participants