feat: Handle NoteExecutionHints in NTB and manage failed notes#1116
feat: Handle NoteExecutionHints in NTB and manage failed notes#1116Mirko-von-Leipzig merged 49 commits intonextfrom
Conversation
|
Still a draft but worth considering at this stage. |
sergerad
force-pushed
the
sergerad-note-management
branch
from
e596651 to
6208f1b
Compare
Mirko-von-Leipzig
left a comment
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
Some nits and suggestions but overall a good implementation.
sergerad
force-pushed
the
sergerad-note-management
branch
from
1ca419b to
24cacad
Compare
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you. I left some comments inline.
Beyond these, I think the main thing we are missing (unless I missed it somehow) is updating the failure count based on the result of the NoteConsumptionChecker. Currently, this check happens inside NtxContext::filter_notes(). There, we try to execute the notes and get the first failed not - but we don't propagate this information anywhere.
This is actually the place where we need to capture failures as most notes that fail to execute will not pass the NoteConsumptionChecker.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left some more comments inline. Most of them are small nits and minor code improvements.
sergerad
force-pushed
the
sergerad-note-management
branch
from
d77d902 to
dcb6d94
Compare
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left one comment for the future inline.
Also, would be good to run client integration tests (especially the ones related to network transactions) against this branch.
| let notes = notes.into_iter().map(Note::from).collect::<Vec<_>>(); | ||
| let (successful, failed) = self.filter_notes(&data_store, notes).await?; | ||
| let executed = Box::pin(self.execute(&data_store, successful)).await?; | ||
| let proven = Box::pin(self.prove(executed)).await?; | ||
| self.submit(proven).await?; | ||
| Ok(()) | ||
| Ok(failed) |
There was a problem hiding this comment.
This is fine for now (no need to change anything) but if self.execute() or self.prove() fail for some reason, failed notes will never be marked as "failed". We can take this into consideration as we refactor this in the future to work more in the actor-model-like fashion.
All passed 👍 |
3 participants
Context
Closes #1071.
The Network Transaction Builder currently does not have any logic to manage notes that have failed. We perform a shuffle of the note list so that we don't get blocked on a forever-failing note.
Changes
InflightNetworkNotestruct to allowAccountStateand NTBStateto keep track of failed notes.Statenote selection logic to consider note failure counts and relevant block numbers.