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

fixed transaction addresses mapping, fixes #1971 #2026

Merged
merged 2 commits into from
Sep 1, 2016
Merged

fixed transaction addresses mapping, fixes #1971 #2026

merged 2 commits into from
Sep 1, 2016

Conversation

debris
Copy link
Collaborator

@debris debris commented Aug 31, 2016

changes:

  • only transactions from canon chain will be stored in mapping TransactionHash -> TransactionAddress
  • tests

@debris debris added the A0-pleasereview 🤓 Pull request needs code review. label Aug 31, 2016
@arkpar
Copy link
Collaborator

arkpar commented Aug 31, 2016

Receipts must be updated as well

@arkpar arkpar added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 31, 2016
@arkpar
Copy link
Collaborator

arkpar commented Aug 31, 2016 •
edited
Loading

Ideally, any info on transactions that are removed from canon chain and never added back should be deleted. But if it make is to complicated, I'm fine with them sticking around.

rphmeier
rphmeier reviewed Aug 31, 2016
View reviewed changes
ethcore/src/blockchain/blockchain.rs
BlockLocation::BranchBecomingCanonChain(ref data) => {
let addresses = data.enacted.iter()
.map(|hash| (hash, self.block_body(hash).unwrap()))
.flat_map(|(hash, bytes)| {
Copy link
Contributor

Choose a reason for hiding this comment

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

should be an expect and the map and flat_map here can probably be unified.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@debris
Copy link
Collaborator Author

debris commented Aug 31, 2016

Receipts must be updated as well

Receipts are stored in hashmap, where the key is block hash (HashMap<BlockHash, BlockReceipt>), so they don't have to be updated.

Ideally, any info on transactions that are removed from canon chain and never added back should be deleted. But if it make is to complicated, I'm fine with them sticking around.

It will make it slightly more complicated and I would prefer to do it in separate pr. The pr would be focus on removing all the unused data from db

@debris debris added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Aug 31, 2016
@arkpar
Copy link
Collaborator

arkpar commented Aug 31, 2016

Right, I was assuming the key for receipts is tx hash

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 31, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 85.447% when pulling 996b4b9 on fixed_transaction_addresses into 25e6a4e on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 85.444% when pulling f5f4736 on fixed_transaction_addresses into 25e6a4e on master.

@debris debris merged commit e159b5f into master Sep 1, 2016
@debris debris mentioned this pull request Sep 5, 2016
Closed
@debris debris deleted the fixed_transaction_addresses branch September 5, 2016 13:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@debris @arkpar @coveralls @rphmeier