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

mitigate refcell conflict in state diffing #2601

Merged
merged 2 commits into from
Oct 13, 2016
Merged

mitigate refcell conflict in state diffing #2601

merged 2 commits into from
Oct 13, 2016

Conversation

rphmeier
Copy link
Contributor

Should fix #2600

I also switched a few uses of borrow_mut to get_mut, which should lead to better overall correctness.
These RefCells are pretty dangerous, and the strange mishmash of interior and actual mutability in this structure can lead to correctness issues, not to mention some slight runtime overhead.

It can probably be refactored a bit to make it safer. Use of RefCell should be considered similar to Option::unwrap.

Also uses RefCell::get_mut in a few places.
@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 12, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 86.465% when pulling 590469e on diff-cell-panic into c9ce25c on master.

@tomusdrw
Copy link
Collaborator

Test to cover the bug would be nice.

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

grumble for test

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

rphmeier commented Oct 13, 2016

Added a test case which is confirmed to panic on master and passes on this branch. There may be other borrowing issues, but it will require significant changes in State, Externalities and maybe Executive to expunge the RefCells here.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 86.505% when pulling 355216b on diff-cell-panic into c9ce25c on master.

@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 13, 2016
@arkpar arkpar merged commit 4581469 into master Oct 13, 2016
arkpar pushed a commit that referenced this pull request Oct 14, 2016
* mitigate refcell conflict in state diffing

Also uses RefCell::get_mut in a few places.

* Add test case
arkpar added a commit that referenced this pull request Oct 14, 2016
* v1.3.8

* mitigate refcell conflict in state diffing (#2601)

* mitigate refcell conflict in state diffing

Also uses RefCell::get_mut in a few places.

* Add test case

* Fixed stalled sync

* Fixed tx queue limit for local transactions (#2616)

* Fixed tx queue limit for local tx

* Fixing test

* Increas gas limit to 20x

* Additional logs when transactions is removed from queue (#2617)

* Database performance tweaks (#2619)
@gavofyork gavofyork deleted the diff-cell-panic branch November 3, 2016 11:51
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. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RPC trace_replayTransaction for stateDiff crashes Parity for some transactions
5 participants