Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Verify sender's balance before importing transaction to queue #746

Merged
merged 14 commits into from Mar 17, 2016

Conversation

tomusdrw
Copy link
Collaborator

  • Additional trace logging

@tomusdrw tomusdrw added the A0-pleasereview 🤓 Pull request needs code review. label Mar 16, 2016
@arkpar
Copy link
Collaborator

arkpar commented Mar 17, 2016

Test fails

@tomusdrw
Copy link
Collaborator Author

Yes, sorry. Fixing it (after changing validation to include full transaction cost balance in test was insufficient).

@arkpar
Copy link
Collaborator

arkpar commented Mar 17, 2016

Could you please check that this does not affect the block propagation logic? It should not send out any blocks (and transactions) while the sync is still active, and I believe this was done by not sending the chain changed event until the sync is complete.

@arkpar arkpar added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 17, 2016
@gavofyork
Copy link
Contributor

ditto. also, right now the client and sync logic is extremely brittle. please place plentiful comments/documentation around any logic changed.

let import = miner.import_transactions(vec![signed_transaction], |a: &Address| client.nonce(a));
let import = miner.import_transactions(vec![signed_transaction], |a: &Address| AccountDetails {
nonce: client.nonce(a),
balance: client.balance(a)
Copy link
Contributor

Choose a reason for hiding this comment

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

missing comma.

@tomusdrw tomusdrw changed the title Inform about every chain change and verify sender's balance before importing transaction to queue Verify sender's balance before importing transaction to queue Mar 17, 2016
@tomusdrw
Copy link
Collaborator Author

After chat with @arkpar I've removed the change in client (it should be in separate pr anyway).

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Mar 17, 2016
@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 17, 2016
@gavofyork
Copy link
Contributor

ok - lgtm then.

gavofyork added a commit that referenced this pull request Mar 17, 2016
Verify sender's balance before importing transaction to queue
@gavofyork gavofyork merged commit a76eb02 into master Mar 17, 2016
@gavofyork gavofyork deleted the tx_queue branch March 17, 2016 12:32
@arkpar arkpar added this to the 1.0 Parity milestone Mar 20, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants