Skip to content
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

DAO soft-fork #2725

Merged
merged 3 commits into from
Jun 24, 2016
Merged

DAO soft-fork #2725

merged 3 commits into from
Jun 24, 2016

Conversation

karalabe
Copy link
Member

@karalabe karalabe commented Jun 22, 2016

This PR is the implementation of the proposed surgical soft-fork to block DAO contracts from sending funds out into uncontrolled environments. All DAO contracts are blocked from moving funds unless those are initiated by the two whitelisted white-hash contracts. This soft fork is a temporary measure that will be reverted after this mess is sorted out.

How to use

If you agree with the soft-fork to temporarily block DAOs from removing blocks, run Geth with --dao-soft-fork, which will signal to the miner to reduce the block gas limit below the 4M threshold, which is needed to pass the fork vote. You don't need to specify anything else, the miner will target Pi million gas for all blocks below the vote threshold. After the vote block passed (irrelevant of the outcome), the old gas dynamics will take its place.

This implements a generic approach to enabling soft forks by allowing
anyone to put in hashes of contracts that should not be interacted from.
This will help "The DAO" in their endevour to stop any whithdrawals from
any DAO contract by convincing the mining community to accept their code
hash.
@robotally
Copy link

robotally commented Jun 22, 2016

Vote Count Reviewers
👍 0
👎 0

Updated: Fri Jun 24 09:58:55 UTC 2016

@codecov-io
Copy link

codecov-io commented Jun 22, 2016

Current coverage is 52.22%

Merging #2725 into develop will increase coverage by 0.08%

@@            develop      #2725   diff @@
==========================================
  Files           217        217          
  Lines         24865      24909    +44   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          12965      13009    +44   
- Misses        10749      10753     +4   
+ Partials       1151       1147     -4   

Powered by Codecov. Last updated by 599e3c7...9b58c2e

@karalabe karalabe force-pushed the obscuren-softfork-dao-2 branch 2 times, most recently from 5c318c5 to a633d54 Compare June 23, 2016 08:50
@karalabe
Copy link
Member Author

@karalabe karalabe force-pushed the obscuren-softfork-dao-2 branch 2 times, most recently from 9707c03 to 244c8f3 Compare June 23, 2016 11:37
common.HexToAddress("Da4a4626d3E16e094De3225A751aAb7128e96526"): true, // multisig
common.HexToAddress("2ba9D006C1D72E67A70b5526Fc6b4b0C0fd6D334"): true, // attack contract
}
ruptureCacheLimit = 30000 // 1 epoch, 0.5 per possible fork
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a specific reason why these are globals?

Copy link
Member Author

Choose a reason for hiding this comment

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

Easily configurable from one location. Nothing else. Beats hard coding them into the middle of the file :)

@karalabe
Copy link
Member Author

@fjl PTAL

@karalabe
Copy link
Member Author

I've squashed this commit, ready for merge. PTAL.

@@ -85,6 +85,11 @@ func exec(env vm.Environment, caller vm.ContractRef, address, codeAddr *common.A
createAccount = true
}

// mark the code hash if the execution is a call, callcode or delegate.
Copy link
Member

Choose a reason for hiding this comment

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

This comment is not very helpful if you don't know the background why some code needs to be marked as special. I would suggest removing this comment line and add a bit more explanation to the docs for Environment.MarkCodeHash why such a feature is available and used for. In execDelegateCall there is no comment for the same logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. I just inherited it from Jeff's code. Will patch it up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed:

// Mark all contracts doing outbound value transfers to allow DAO filtering.

}
// Verify if the DAO soft fork kicks in
if blockRuptureCodes {
if recipient := tx.To(); recipient == nil || !ruptureWhitelist[*recipient] {
Copy link
Member

Choose a reason for hiding this comment

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

Is it just to or also from?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to. These are not plain addresses, rather multisig contracts. So to whitelist interaction with these contracts, the transaction to fields needs to be actually checked.

if err != nil {
return nil, nil, nil, err
}

// Check whether the DAO needs to be blocked or not

Choose a reason for hiding this comment

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

Since miners only activate the soft fork if DAOSoftFork is set, shouldn't this code also check for it? Otherwise we make the soft fork optional for the block-to-mine code but mandatory for all block validation.

Copy link
Member Author

@karalabe karalabe Jun 24, 2016

Choose a reason for hiding this comment

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

Indeed (mandatory for all validation) and that is the point of the soft fork. If the majority votes to pass it, others voting against it will also honor it and keep the network together. Otherwise the network will just hard split into two.

Copy link

Choose a reason for hiding this comment

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

The point is that we should either check for DAOSoftFork here (and in the other places where the soft fork is enacted) or not have the flag at all in the soft fork version of the code. Currently it's inconsistent, as there's then no point in running this version as a miner if one does not want to vote for the soft fork.

@fjl
Copy link
Contributor

fjl commented Jun 24, 2016

👍

@karalabe karalabe merged commit 848dec3 into ethereum:develop Jun 24, 2016
@obscuren obscuren removed the review label Jun 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants