This repository has been archived by the owner on Jul 27, 2022. It is now read-only.
Problem:(CRO-524) Client may create transactions with potentially spent utxos #701
Conversation
This file contains 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
linfeng-crypto
requested review from
calvinaco,
devashishdxt and
tomtau
as code owners
Codecov Report
@@ Coverage Diff @@
## master #701 +/- ##
==========================================
+ Coverage 69.12% 69.18% +0.05%
==========================================
Files 132 132
Lines 16896 17184 +288
==========================================
+ Hits 11680 11888 +208
- Misses 5216 5296 +80
|
devashishdxt
suggested changes
Dec 18, 2019
|
|
||
| wallet_client.broadcast_transaction(&transaction)?; | ||
|
|
||
| if let Some(tp) = tx_pending { | ||
| wallet_client.update_tx_pending_state(&name, &passphrase, transaction.tx_id(), tp)?; |
There was a problem hiding this comment.
Updating pending_state looks like business logic which should be a part of client-core instead of client-cli. Maybe inside broadcast_transaction()?
There was a problem hiding this comment.
I don't think so, cause broadcast_transaction is a solo logic that just broadcast the tx to tendermint and not needs name and passphrase, updating the wallet state is also a part of wallet client jobs like broadcasting the tx.
| @@ -38,7 +40,7 @@ pub trait WalletTransactionBuilder: Send + Sync { | |||
| outputs: Vec<TxOut>, | |||
| return_address: ExtendedAddr, | |||
| attributes: TxAttributes, | |||
| ) -> Result<TxAux>; | |||
| ) -> Result<(TxAux, Vec<TxoPointer>, Coin)>; | |||
There was a problem hiding this comment.
Can you update documentation with details of returned values?
| @@ -461,6 +461,26 @@ where | |||
| self.tendermint_client | |||
| .broadcast_transaction(&tx_aux.encode()) | |||
| } | |||
| #[inline] | |||
There was a problem hiding this comment.
Suggested change
| #[inline] |
| Ok(current_block_height) | ||
| } | ||
|
|
||
| #[inline] |
There was a problem hiding this comment.
Suggested change
| #[inline] |
tomtau
suggested changes
Dec 19, 2019
linfeng-crypto
force-pushed
the
CRO-524
branch
2 times, most recently
from
7ced010 to
7896c80
Compare
|
bors r+ |
bors bot
added a commit
that referenced
this pull request
Dec 20, 2019
701: Problem:(CRO-524) Client may create transactions with potentially spent utxos r=tomtau a=linfeng-crypto solution: - add a `pending_transaction` in wallet state, it's a BTreepMap of `txid` -> `pending_info` - the pending_info includes the `current block height` when the tx broadcast, the returned coin amount and the used `inputs` - after broadcast a transaction, we update the `pending_transaction` - when make a new transaction, we select the inputs which are in `unspent_transactions` and not in `pending_transactions` - we remove the `txid` from the `pending_transaction` when we find it in `sync` - we also remove the `txid` after `sync` finished if it can not be found in the latest `BLOCK_HEIGHT_ENSURE`(which is 50) blocks after it broadcast - change the wallet balance into 3 types: `total`, `pending`, `available`, we can only use the coins amount which is in `available` when we make a new transaction. Co-authored-by: linfeng <linfeng@crypto.com>
Build failed |
tomtau
suggested changes
Dec 20, 2019
There was a problem hiding this comment.
seems the integration tests fail:
9 failing
6231
6232 1) Wallet Auto-sync
6233 can auto-sync unlocked wallets:
6234 AssertionError: Sender balance should be deducted by transfer amount: expected { Object (available, pending, ...) } to equal 'NaN'
6235 at Context.<anonymous> (test/hdrestore-auto-sync.test.ts:168:43)
6236 at process._tickCallback (internal/process/next_tick.js:68:7)
6237
6238
6239 2) Wallet transaction
6240 "before all" hook in "Wallet transaction":
6241 Error: Error when withdrawing all unbonded stake: Validation error: Failed to validate staking account: output with no credited value, origin: None
6242 at Object.exports.asyncMiddleman (test/core/utils.ts:81:9)
6243 at process._tickCallback (internal/process/next_tick.js:68:7)
6244
6245
6246 3) Wallet Auto-sync
6247 "before all" hook for "can auto-sync unlocked wallets":
6248 Error: Error when withdrawing all unbonded stake: Validation error: Failed to validate staking account: output with no credited value, origin: None
6249 at Object.exports.asyncMiddleman (test/core/utils.ts:81:9)
6250 at process._tickCallback (internal/process/next_tick.js:68:7)
6251
6252
6253 4) Wallet management
6254 "before all" hook for "can restore hd-wallet with specified name":
6255 Error: Error when withdrawing all unbonded stake: Validation error: Failed to validate staking account: output with no credited value, origin: None
6256 at Object.exports.asyncMiddleman (test/core/utils.ts:81:9)
6257 at process._tickCallback (internal/process/next_tick.js:68:7)
6258
6259
6260 5) Wallet transaction
6261 "before all" hook in "Wallet transaction":
6262 Error: Error when withdrawing all unbonded stake: Validation error: Failed to validate staking account: output with no credited value, origin: None
6263 at Object.exports.asyncMiddleman (test/core/utils.ts:81:9)
6264 at process._tickCallback (internal/process/next_tick.js:68:7)
6265
6266
6267 6) Staking
6268 "before all" hook for "unbond of same amount and nonce from different account should have different txid":
6269 Error: Error when withdrawing all unbonded stake: Validation error: Failed to validate staking account: output with no credited value, origin: None
6270 at Object.exports.asyncMiddleman (test/core/utils.ts:81:9)
6271 at process._tickCallback (internal/process/next_tick.js:68:7)
6272
6273
6274 7) Wallet Auto-sync
6275 "before all" hook for "can auto-sync unlocked wallets":
6276 Error: Error when withdrawing all unbonded stake: Validation error: Failed to validate staking account: output with no credited value, origin: None
6277 at Object.exports.asyncMiddleman (test/core/utils.ts:81:9)
6278 at process._tickCallback (internal/process/next_tick.js:68:7)
6279
6280
6281 8) Wallet management
6282 "before all" hook for "cannot access un-existing wallet":
6283 Error: Error when withdrawing all unbonded stake: Validation error: Failed to validate staking account: output with no credited value, origin: None
6284 at Object.exports.asyncMiddleman (test/core/utils.ts:81:9)
6285 at process._tickCallback (internal/process/next_tick.js:68:7)
6286
6287
6288 9) Wallet transaction
6289 "before all" hook in "Wallet transaction":
6290 Error: Error when withdrawing all unbonded stake: Validation error: Failed to validate staking account: output with no credited value, origin: None
|
Because the balance format changed from |
| .print_stdout() | ||
| .chain(|| (ErrorKind::IoError, "Unable to print table"))?; | ||
|
|
||
| success(&format!("Wallet balance: \n {:?}", balance)); |
There was a problem hiding this comment.
Suggested change
| success(&format!("Wallet balance: \n {:?}", balance)); |
linfeng-crypto
force-pushed
the
CRO-524
branch
3 times, most recently
from
97b082f to
ea4a85e
Compare
calvinaco
suggested changes
Dec 27, 2019
integration-tests/client-rpc/test/hdrestore-transaction.test.ts
Outdated
Show resolved
Hide resolved
linfeng-crypto
force-pushed
the
CRO-524
branch
2 times, most recently
from
a51007f to
55476d3
Compare
tomtau
suggested changes
Jan 2, 2020
linfeng-crypto
force-pushed
the
CRO-524
branch
from
55476d3 to
755095a
Compare
|
bors try+ |
linfeng-crypto
force-pushed
the
CRO-524
branch
2 times, most recently
from
b4fddea to
96f2b77
Compare
|
bors try |
tryAlready running a review |
|
bors r+ |
bors bot
added a commit
that referenced
this pull request
Jan 3, 2020
701: Problem:(CRO-524) Client may create transactions with potentially spent utxos r=tomtau a=linfeng-crypto solution: - add a `pending_transaction` in wallet state, it's a BTreepMap of `txid` -> `pending_info` - the pending_info includes the `current block height` when the tx broadcast, the returned coin amount and the used `inputs` - after broadcast a transaction, we update the `pending_transaction` - when make a new transaction, we select the inputs which are in `unspent_transactions` and not in `pending_transactions` - we remove the `txid` from the `pending_transaction` when we find it in `sync` - we also remove the `txid` after `sync` finished if it can not be found in the latest `BLOCK_HEIGHT_ENSURE`(which is 50) blocks after it broadcast - change the wallet balance into 3 types: `total`, `pending`, `available`, we can only use the coins amount which is in `available` when we make a new transaction. Co-authored-by: linfeng <linfeng@crypto.com>
Build failed |
|
seems there's some problem in the integration tests: |
tryBuild failed |
…nt utxos solution: - add a `pending_transaction` in wallet state, it's a BTreepMap of `txid` -> `pending_info` - the pending_info includes the `current block height` when the tx broadcast, the returned coin amount and the used `inputs` - after broadcast a transaction, we update the pending_info - when make a new transaction, we select the inputs which are in `unspent_transactions` and not in `pending_transactions` - we remove the `txid` from the `pending_transaction` when we find it in `sync` - we also remove the `txid` after `sync` finished if it can not be found in the latest `BLOCK_HEIGHT_ENSURE`(which is 50) blocks after it broadcast - change the wallet balance into 3 types: `total`, `pending`, `available`, we can only use the coins amount which is in `available` when we make a new transaction. - make integration test worked
linfeng-crypto
force-pushed
the
CRO-524
branch
from
96f2b77 to
ee28a09
Compare
|
bors retry |
bors bot
added a commit
that referenced
this pull request
Jan 6, 2020
701: Problem:(CRO-524) Client may create transactions with potentially spent utxos r=tomtau a=linfeng-crypto solution: - add a `pending_transaction` in wallet state, it's a BTreepMap of `txid` -> `pending_info` - the pending_info includes the `current block height` when the tx broadcast, the returned coin amount and the used `inputs` - after broadcast a transaction, we update the `pending_transaction` - when make a new transaction, we select the inputs which are in `unspent_transactions` and not in `pending_transactions` - we remove the `txid` from the `pending_transaction` when we find it in `sync` - we also remove the `txid` after `sync` finished if it can not be found in the latest `BLOCK_HEIGHT_ENSURE`(which is 50) blocks after it broadcast - change the wallet balance into 3 types: `total`, `pending`, `available`, we can only use the coins amount which is in `available` when we make a new transaction. Co-authored-by: linfeng <linfeng@crypto.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
solution:
pending_transactionin wallet state, it's a BTreepMap oftxid->pending_infocurrent block heightwhen the tx broadcast, the returned coin amount and the usedinputspending_transactionunspent_transactionsand not inpending_transactionstxidfrom thepending_transactionwhen we find it insynctxidaftersyncfinished if it can not be found in the latestBLOCK_HEIGHT_ENSURE(which is 50) blocks after it broadcasttotal,pending,available, we can only use the coins amount which is inavailablewhen we make a new transaction.