-
Notifications
You must be signed in to change notification settings - Fork 0
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
Altered behaviour of GetTransactionStatus #2170
base: main
Are you sure you want to change the base?
Altered behaviour of GetTransactionStatus #2170
Conversation
|
Branch | max/fix/2167-incorrect-status-returned-by-gettransactionstatus |
Testbed | self-hosted |
Click to view all benchmark results
Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
---|---|---|---|
full-blocks-erc20-transfers/full-blocks-erc20-transfers | 📈 view plot 🚷 view threshold | 956.87(-19.19%)Baseline: 1,184.04 | 1,577.04 (60.67%) |
full-blocks-evm-transfers/full-blocks-evm-transfers | 📈 view plot 🚷 view threshold | 372.64(-18.38%)Baseline: 456.53 | 619.56 (60.15%) |
full-blocks-zil-transfers/full-blocks-zil-transfers | 📈 view plot 🚷 view threshold | 4,050.00(+2.35%)Baseline: 3,957.14 | 5,323.65 (76.08%) |
process-empty/process-empty | 📈 view plot 🚷 view threshold | 9.75(+5.24%)Baseline: 9.26 | 11.73 (83.13%) |
See discussion in #2167 |
- rewrite of status code calculation - docs updates - now handles transactions that aren't in a block more correctly
…ttransactionstatus
- Removed comments indicating deprecated options in status enum - Refactored state decision code from api/types/zil.rs to api/zilliqa.rs - Fixed finalization checking logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I am confused about the interaction between receipt
and block
. I might be misunderstanding, in which case feel free to ignore me and merge.
- refactored handling of missing receipts - cut down on a vec iteration in looking up transaction pending/queued
…ttransactionstatus
state: &State, | ||
txn: &VerifiedTransaction, | ||
) -> Result<Option<PendingOrQueued>> { | ||
if txn.tx.nonce().ok_or(anyhow!("Transaction missing nonce"))? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also need to check if the hash is included in the mempool here. Otherwise all transactions with the right nonce will be 'pending' even if they aren't in the pool at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in something like self.hash_to_index.contains_key(&txn.hash) && txn.tx.nonce().ok_or(anyhow!("Transaction missing nonce"))? == state.get_account(txn.signer)?.nonce
? I'm not sure I get the relationship between a transaction nonce and an account nonce so I'm not sure exactly what the line is doing.
…ttransactionstatus
I'm very open to different logic if anyone knows better exactly what the behaviour should be here.
Intended to fix #2167