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: handle receiver cancelling an inbound transaction that is later received #3177

Merged

Conversation

philipr-za
Copy link
Contributor

@philipr-za philipr-za commented Aug 10, 2021

Description

This PR addresses the following scenario spotted by @sdbondi :

  • NodeA sends to nodeB(offline)
  • NodeA goes offline
  • NodeB receives tx, and cancels it (weird I know)
  • NodeA comes online and broadcasts the transaction
  • NodeB is not aware of the transaction, transaction complete for NodeA

This is handled by adding logic that if a FinalizedTransaction is received with no active Receive Protocols that the database is checked if there is a matching cancelled inbound transaction from the same pubkey. If there is the receiver might as well restart that protocol and accept the finalized transaction.

This required adding in functionality to the Transaction and Output Manager services to reinstate a cancelled inbound transaction

How Has This Been Tested?

A cucumber test is provided to test this case.

Unit tests provided for new functionality in the Output Manager and Transaction Services

Checklist:

  • I'm merging against the development branch.
  • I have squashed my commits into a single commit.

@philipr-za philipr-za changed the title Handle receiver cancelling an inbound transaction that is later received [wallet] Handle receiver cancelling an inbound transaction that is later received Aug 10, 2021
@philipr-za philipr-za force-pushed the philip-cucumber-saf-cancel branch 6 times, most recently from 5beb8e8 to 637fab7 Compare August 11, 2021 11:48
@aviator-app
Copy link
Contributor

aviator-app bot commented Aug 11, 2021

Waiting for approval before queuing

stringhandler
stringhandler previously approved these changes Aug 11, 2021
@aviator-app
Copy link
Contributor

aviator-app bot commented Aug 11, 2021

PR queued successfully. Your position in queue is: 5

@stringhandler stringhandler changed the title [wallet] Handle receiver cancelling an inbound transaction that is later received fix: handle receiver cancelling an inbound transaction that is later received Aug 11, 2021
@aviator-app
Copy link
Contributor

aviator-app bot commented Aug 11, 2021

PR is on top of the queue now

@aviator-app
Copy link
Contributor

aviator-app bot commented Aug 11, 2021

PR failed to merge with reason: Some CI status(es) failed
Failed CI(s): ci/circleci: run-integration-tests

@philipr-za philipr-za force-pushed the philip-cucumber-saf-cancel branch 3 times, most recently from 74929e3 to 4ec7cfa Compare August 12, 2021 13:40
This PR addresses the following scenario spotted by @stanimal:

 - NodeA sends to nodeB(offline) 
- NodeA goes offline 
- NodeB receives tx, and cancels it (weird I know) 
- NodeA comes online and broadcasts the transaction 
- NodeB is not aware of the transaction, transaction complete for NodeA

This is handled by adding logic that if a FinalizedTransaction is received with no active Receive Protocols that the database is checked if there is a matching cancelled inbound transaction from the same pubkey. If there is the receiver might as well restart that protocol and accept the finalized transaction.

A cucumber test is provided to test this case.

This required adding in functionality to the Transaction and Output Manager service to reinstate a cancelled inbound transaction, unit tests provided for that.
@philipr-za philipr-za force-pushed the philip-cucumber-saf-cancel branch from 4ec7cfa to a16d04b Compare August 16, 2021 07:50
@stringhandler stringhandler merged commit c79e53c into tari-project:development Aug 17, 2021
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