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

Fix transaction logs incorrectly marked successful #1027

Merged

Conversation

nick-mobilecoin
Copy link
Contributor

Previously transaction_logs were marked successful if the input TXO(s)
were spent according to the block chain. This was problematic when
multiple transaction_logs were created that utilized the same input
TXOs. These transaction_logs using the same input TXOs could happen
when two clients were talking to the same Full-Service instance and
those clients were using two step build and submit:

  1. Client_a sends build_transaction, input TXO_n is used
  2. Client_b sends build_transaction, input TXO_n is also used
  3. Client_a submit_transaction
  4. Client_b submit_transaction before the block chain knows that the
    Client_a transaction exists and will consume TXO_n. The blockchain
    will still reject this transaction eventually.
  5. Previously, Full-Service would see that input TXO_n has been consumed
    and will mark all submitted transaction_logs that used TXO_n as
    successful.

Now transaction_logs are marked successful based on if their output
TXO(s) land on the blockchain. This ensures that only the transaction
log that the blockchain accepted will be marked as successful.

Copy link
Collaborator

@holtzman holtzman left a comment

Choose a reason for hiding this comment

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

Looks good!

I think failing a tranasction_log where the input is spent and the output is not present in the same block would be good for the case where 2x full-service with the same account keys are running together, which is a situation that inspired this PR.

That said, this PR fixes a bug whereas that would be an improvement, allowing the full-service with the failed tx to recognize it sooner, so could be the subject of a future PR.

Base automatically changed from nick/consolidate-sync-account-next-chunk to main December 3, 2024 21:34
Previously `transaction_log`s were marked successful if the input TXO(s)
were spent according to the block chain. This was problematic when
multiple `transaction_log`s were created that utilized the same input
TXOs. These `transaction_log`s using the same input TXOs could happen
when two clients were talking to the same Full-Service instance and
those clients were using two step build and submit:

1. Client_a sends `build_transaction`, input TXO_n is used
2. Client_b sends `build_transaction`, input TXO_n is also used
3. Client_a `submit_transaction`
4. Client_b `submit_transaction` before the block chain knows that the
   Client_a transaction exists and will consume `TXO_n`. The blockchain
   will still reject this transaction eventually.
5. Previously, Full-Service would see that input TXO_n has been consumed
   and will mark **all** submitted `transaction_log`s that used TXO_n as
   successful.

Now `transaction_log`s are marked successful based on if their output
TXO(s) land on the blockchain. This ensures that only the transaction
log that the blockchain accepted will be marked as successful.
@nick-mobilecoin nick-mobilecoin force-pushed the nick/fix-transaction-logs-incorrectly-marked-successful branch from ebd3a46 to e4b40bf Compare December 3, 2024 21:36
@nick-mobilecoin nick-mobilecoin enabled auto-merge (squash) December 3, 2024 21:37
@nick-mobilecoin nick-mobilecoin merged commit cb2f46e into main Dec 3, 2024
24 checks passed
@nick-mobilecoin nick-mobilecoin deleted the nick/fix-transaction-logs-incorrectly-marked-successful branch December 3, 2024 22:22
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.

2 participants