ENT-14451: Ensure that a CorDapp is present in cordapps directory when choosing an attachment for a missing class dependency #8014
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tically too" This reverts commit fc0542b.
|
Please add back the test case that checks if multiple legacy cordapps contain the same class; the transaction will be built correctly. Your current implementation of course allows that, but this is an important behaviour from a backwards compatibility point of view. You should also include a test case that verifies your approach of sorting by first the version and then the ID, and make sure it works as expected. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Two separate problems were flagged by @Tagada14 :
When building a transaction, the TransactionBuilder attempts to verify a transaction, and if it encounters a missing class dependency, it looks for it in the attachments and adds the attachment with the missing class to the transaction. However, it considers all of the attachments from the Attachment Storage rather than the CorDapps present in the node. This allows it to add an arbitrary trusted attachment that contains the missing class, even a legacy one!Fix:
Default: true → fallback to database allowed
false → only installed CorDapps are considered; database attachments ignored
Moreover, it doesn't check if multiple attachments contain said class; it simply chooses the first one. Because of this, the attachment it chooses is runtime-dependent and non-deterministic. With the same set of attachments in the storage, one node may attach CorDapp A with the missing class and the other CorDapp B.This has been fixed by improving the sorting mechanism when returning attachments - now attachments are additionally (apart from primary sorting by version descending) sorted by their IDs, which ensures deterministic order of returned attachments. For example, if we have two attachments with the same version, they'll be ordered alphabetically, not returned in random order as before.
Credits to @Tagada14 for finding the above bug and suggesting a good solution in #7994.
PR Checklist:
https://docs.r3.com/release-notes.html)?Thanks for your code, it's appreciated! :)