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

feat!: add less aggressive txn cancelling #3904

Conversation

hansieodendaal
Copy link
Contributor

@hansieodendaal hansieodendaal commented Mar 8, 2022

Description

  • Added less aggressive transaction canceling, i.e. removed automatic transaction cancelling, when:
    • a wallet has difficulty discovering the receiving party (for direct P2P comms);
    • a wallet has difficulty discovering base node peers that could be used to send the transaction via store and forward;
    • tor is not available.
  • Updated the wallet ffi with the new callbacks.
  • Added a cucumber test to test sending transactions while offline.

Users will now manually cancel transactions stuck in pending due to the wallet continuing to be being offline.

Note: Wallet ffi interface changed.

Motivation and Context

During a recent semi-stress test [report here] adverse connectivity and/or discovery symptoms caused ~55% of transactions to be abandoned. Attempted transactions should not be canceled if they failed to be sent both directly and via store and forward; a peer-to-peer connection may fail for valid reasons but failing to submit a transaction to a base node for store and forward delivery does not warrant the transaction to be canceled.

How Has This Been Tested?

System-level testing
Unit tests
Cucumber test: Scenario: Wallet send transactions while offline

@hansieodendaal hansieodendaal force-pushed the ho_less_aggressive_txn_cancelling branch 5 times, most recently from 730924e to 9bf72fb Compare March 19, 2022 07:00
@hansieodendaal hansieodendaal force-pushed the ho_less_aggressive_txn_cancelling branch from 9bf72fb to 035d7ee Compare March 22, 2022 09:54
@hansieodendaal hansieodendaal changed the title [wip] feat: add less aggressive txn cancelling feat!: add less aggressive txn cancelling Mar 22, 2022
@hansieodendaal hansieodendaal force-pushed the ho_less_aggressive_txn_cancelling branch from 035d7ee to ee46b1f Compare March 23, 2022 08:16
Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

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

Looks fine. I believe all the send scenarios are catered for in the tests? Cucumber or Rust?

Just think the FFI interface is not as clean as it could be.

base_layer/wallet_ffi/src/lib.rs Outdated Show resolved Hide resolved
@hansieodendaal hansieodendaal force-pushed the ho_less_aggressive_txn_cancelling branch 2 times, most recently from f43b27b to 5f7989c Compare March 31, 2022 11:58
- Added less aggressive transaction cancelling when a wallet has difficulty
discovering the receiving party (for direct P2P comms) or base node peers
that could be used to send the transaction via store and forward.
- Updated the wallet ffi with the new callbacks.
- Added a cucuumber test to test sending transactions while offline.
@hansieodendaal hansieodendaal force-pushed the ho_less_aggressive_txn_cancelling branch from 5f7989c to 7fc57c5 Compare March 31, 2022 15:37
@hansieodendaal
Copy link
Contributor Author

Looks fine. I believe all the send scenarios are catered for in the tests? Cucumber or Rust?

Yes, I added a special cucumber test to test the queueing as well.

@hansieodendaal hansieodendaal force-pushed the ho_less_aggressive_txn_cancelling branch from 7fc57c5 to b996298 Compare April 1, 2022 04:59
@aviator-app aviator-app bot merged commit 40bedde into tari-project:development Apr 1, 2022
@hansieodendaal hansieodendaal deleted the ho_less_aggressive_txn_cancelling branch April 1, 2022 07:38
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.

3 participants