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

Multisignature and OP_EVAL support #669

Merged
merged 12 commits into from Dec 20, 2011

Conversation

@gavinandresen
Copy link
Contributor

commented Nov 28, 2011

This implements BIPS 11, 12, and 13 :

  • OP_CHECKMULTISIG transactions supported, for up to 3 public keys, as 'standard' transactions
  • OP_EVAL (same opcode as OP_NOP1) as a new opcode
  • New OP_EVAL-based 'standard' transaction type and Bitcoin address
  • Blocks mined with this patch will have the string "OP_EVAL" in the coinbase scriptSig, so we can tell when a majority of miners support it.
  • New RPC command to add a multisignature-required address to the wallet: addmultisigaddress <nrequired> <'["key1",...]'> [account] (enabled only for use on the -testnet for now)
  • The 'validateaddress' RPC command shows the full public key of addresses in your wallet
  • Internal changes so if you own all the private keys of a multisignature transaction, then you are able to spend the transaction (and the amount shows up in your balance, and the transaction is listed in listtransactions output)

There is still a lot of work to be done to get multi-device transaction authorization or multi-party escrow; in particular, this pull doesn't include any support for gathering transaction signatures from multiple places or showing the user transactions that they are involved in but can't spend without getting more signatures from other devices/people. It just implements the lowest-level support, along with the bare minimum needed to test to make sure the lowest-level stuff is working properly.

To test / play with:

  1. Run bitcoind -daemon -testnet
  2. Get public keys from 2 or 3 new bitcoin addresses-- e.g. run this twice: ./bitcoind -testnet validateaddress $(./bitcoind getnewaddress)
  3. Generate a new multisignature address using the public keys from validateaddress-- ./bitcoind addmultisigaddress 2 '["...public_key_1...","...public_key_2..."]'
  4. Send funds to that address -- ./bitcoind sendtoaddress ...result of addmultisigaddress... 11.11

Step (4) can be done from another ./bitcoind, either on another machine or same machine, different -datadir, as long as it is running this patch. The coins should show up in the wallet, be listed in listransactions, and you should be able to spend them as if they were single-signature transactions.

Step (2) could be done on two or three different machines, but without more work you'll have no way to spend coins sent to the resulting multisignature address.

@mikehearn

View changes

src/db.h Outdated
@@ -420,6 +420,18 @@ public:
return Write(std::make_pair(std::string("mkey"), nID), kMasterKey, true);
}

bool ReadCScript(const uint160 &hash, std::vector<unsigned char>& data)

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

Maybe add comments in keystore.h and db.h indicating why you might want to store scripts keyed by hash in your wallet, as this is not obvious at all given the readers basic mental model of how Bitcoin works.

@mikehearn

View changes

src/keystore.h Outdated
@@ -28,17 +28,23 @@ public:
// This may succeed even if GetKey fails (e.g., encrypted wallets)
virtual bool GetPubKey(const CBitcoinAddress &address, std::vector<unsigned char>& vchPubKeyOut) const;

virtual bool AddCScript(const uint160 &hash, const std::vector<unsigned char>& data) =0;

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

Same here, consider comments explaining what this is all about and why scripts are keyed by hash. Also, the methods say "AddCScript" but actually take an arbitrary byte buffer - which do you want, scripts or any data? Be consistent.

@mikehearn

View changes

src/keystore.h Outdated

// Basic key store, that keeps keys in an address->secret map
class CBasicKeyStore : public CKeyStore
{
protected:
KeyMap mapKeys;
DataMap mapData;

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

mapData is not a very descriptive name, don't all maps contain data?

This comment has been minimized.

Copy link
@gavinandresen

gavinandresen Dec 1, 2011

Author Contributor

I hate naming things... must be my Australian roots. I'll call it Bruce.

@mikehearn

View changes

src/main.cpp Outdated
if (txin.scriptSig.size() > 500)
return error("nonstandard txin, size %d\n", txin.scriptSig.size());
if (!txin.scriptSig.IsPushOnly())
return error("nonstandard txin: %s", txin.scriptSig.ToString().c_str());

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

Maybe state explicitly in these messages why they are considered non-standard (too large, opcodes present)

@mikehearn

View changes

src/main.cpp Outdated
// expensive-to-check-upon-redemption script like:
// DUP CHECKSIG DROP ... repeated 100 times... OP_1
//
bool CTransaction::IsStandardInputs(std::map<uint256, std::pair<CTxIndex, CTransaction> > mapInputs) const

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

::AreInputsStandard?

Consider commenting (or using a typedef) that the map key is a tx hash

@mikehearn

View changes

src/main.cpp Outdated

vector<vector<unsigned char> > vSolutions;
txntype whichType;
if (!Solver(txPrev.vout[vin[i].prevout.n].scriptPubKey, whichType, vSolutions))

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

This is a fairly complex expression, comment what txPrev.vout[vin[i].prevout.n] refers to here (I know it's obvious if you think about it but it's easier to read....)

This comment has been minimized.

Copy link
@gavinandresen

gavinandresen Dec 1, 2011

Author Contributor

That (and several of your other line comments here) is SatoshiCode, just moved around. But you're right, this is a good opportunity to make it easier to read....

@mikehearn

View changes

src/main.cpp Outdated
vector<vector<unsigned char> > vSolutions;
txntype whichType;
if (!Solver(txPrev.vout[vin[i].prevout.n].scriptPubKey, whichType, vSolutions))
return false;

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

return error("...")? same for other locations in this function?


// Read txPrev
CTransaction& txPrev = inputsRet[prevout.hash].second;
if (!fFound || txindex.pos == CDiskTxPos(1,1,1))

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

What does the magic 1,1,1 mean, maybe this should be a global constant?

This comment has been minimized.

Copy link
@gavinandresen

gavinandresen Dec 1, 2011

Author Contributor

Darn good question. More SatoshiCode (in this case, I think just the indentation level changed).

This comment has been minimized.

Copy link
@mikehearn

mikehearn Dec 1, 2011

Contributor

Yup, sorry, I realize it's not yours. Just might as well fix up minor things like this whilst there is attention on a particular part of the code.

// To avoid being on the short end of a block-chain split,
// interpret OP_EVAL as a NO_OP until blocks with timestamps
// after opevaltime:
int64 nEvalSwitchTime = GetArg("opevaltime", 1328054400); // Feb 1, 2012

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

Would it be worth allowing the default time to be adjusted using a message signed by you, embedded in a block coinbase?

Alternatively, calculate it dynamically based on the frequency of the coinbase markers?

This comment has been minimized.

Copy link
@gavinandresen

gavinandresen Dec 1, 2011

Author Contributor

Luke-Jr was big on doing something like that. I don't think the extra code complication is worth it; this is the Simplest Possible Thing That Will Work. Keeping track of frequency, or a signed message, means storing the message somewhere, or storing the frequency somewhere (or recomputing it in LoadBlockChain and recomputing it whenever there's a new block or a block chain reorg, etc etc etc).

@mikehearn

View changes

src/main.cpp Outdated
@@ -2862,6 +2992,12 @@ void IncrementExtraNonce(CBlock* pblock, CBlockIndex* pindexPrev, unsigned int&
++nExtraNonce;
pblock->vtx[0].vin[0].scriptSig = CScript() << pblock->nTime << CBigNum(nExtraNonce);
pblock->hashMerkleRoot = pblock->BuildMerkleTree();

// Put "OP_EVAL" in the coinbase so everybody can tell when

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

That's rather verbose. Why not just have some kind of code prefix like "FM" and then 16 feature bits, in case you want to do this again in future?

This comment has been minimized.

Copy link
@gavinandresen

gavinandresen Dec 1, 2011

Author Contributor

Again, Simplest Possible Thing that will work.

The release after OP_EVAL has majority mining support this code will be removed-- I assume the big mining pools aren't going to remove support for it once they add support for it. The next time with have an OP_EVAL-like change, we'll lobby miners to put some other string in their coinbases....

switch (opcode)
{
// push value
case OP_0 : return "0";

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

This sort of table is often done with a macro to avoid the possibility of typos.

@mikehearn

View changes

src/script.cpp Outdated
CScript subscript(vchScript.begin(), vchScript.end());
popstack(stack);

// Codeseparators not allowed

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

Why forbid them?

This comment has been minimized.

Copy link
@gavinandresen

gavinandresen Dec 1, 2011

Author Contributor

Because they don't make sense inside OP_EVAL'ed code.

Example:
scriptPubKey: DUP HASH160 ...hash of serialized script... EQUALVERIFY EVAL

Redeemed in a transaction that has:
scriptSig: serialized(stuff... CODESEPARATOR CHECKSIG)

So: CHECKSIG takes the part of the scriptPubKey from the last CODESEPARATOR to the end of the scriptPubKey and replaces the scriptSig with that.

But there is no CODESEPARATOR in the scriptPubKey. It is buried inside the scriptSig. The scriptSig that is signed will be DUP HASH160 <hash_of_serialized_script> EQUALVERIFY EVAL

I suppose OP_EVAL could interact with OP_CHECKSIG so the scriptPubKey is rewritten to "expand out" all the OP_EVALs somehow before evaluation so CODESEPARATORS inside the EVAL would make sense... but since I don't see a use for CODESEPARATOR and since that would add quite a lot of extra code and complication just disallowing CODESEPARATOR inside EVAL'd scripts seems like the correct thing to do.

This comment has been minimized.

Copy link
@mikehearn

mikehearn Dec 1, 2011

Contributor

Makes sense. Best to put the explanation in a comment rather than a code review thread though.

@mikehearn

View changes

src/script.cpp Outdated
@@ -70,20 +70,187 @@ static inline void popstack(vector<valtype>& stack)
}


bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, const CTransaction& txTo, unsigned int nIn, int nHashType)
const char* GetTxnTypeName(txntype t)

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

These are actually types of scriptPubKeys, not transactions

This comment has been minimized.

Copy link
@gavinandresen

gavinandresen Dec 1, 2011

Author Contributor

Naming again.... GetScriptPubKeyTypeName ? Is there a better term for "part of a transaction that specifies the conditions necessary to redeem" ?

This comment has been minimized.

Copy link
@mikehearn

mikehearn Dec 1, 2011

Contributor

GetOutputType?

@mikehearn

View changes

src/script.cpp Outdated
{
nRequiredRet = vSolutions.front()[0];
int n = vSolutions.back()[0];
for (vector<valtype>::const_iterator it = vSolutions.begin()+1; it != vSolutions.begin()+vSolutions.size()-1; it++)

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

this might be clearer using integer indexs

@mikehearn

View changes

src/test/script_op_eval_tests.cpp Outdated
BOOST_CHECK(VerifyScript(txTo.vin[0].scriptSig, txFrom.vout[0].scriptPubKey, txTo, 0, nUnused, 0, false));

// After eval switchover, it should be considered invalid:
SetMockTime(nEvalSwitchover);

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

Do you need to reset the mock time afterwards? Don't recall how this works.

This comment has been minimized.

Copy link
@gavinandresen

gavinandresen Dec 1, 2011

Author Contributor

The test fixture resets mock time between tests. Although the mock time doesn't matter any more-- a previous refactoring added Yet Another argument to VerifyScript that controls whether it interprets OP_EVAL as a no-op or not. I'll fix the test case.

@mikehearn

View changes

src/wallet.cpp Outdated
CBitcoinAddress address;
if (ExtractAddress(txout.scriptPubKey, this, address) && !address.IsScript())
CRITICAL_BLOCK(cs_wallet)
if (!mapAddressBook.count(address))

This comment has been minimized.

Copy link
@mikehearn

mikehearn Nov 30, 2011

Contributor

That's a bit confusing. Isn't there a simpler way to phrase this check?

@luke-jr

This comment has been minimized.

Copy link
Member

commented Dec 3, 2011

NACK

9db95d3 introduces a regression: when you send-to-self, and have to pay a fee, instead of the usual send/receive pair in listtransactions, we now get a second 'send' instead of the 'receive'. This 'send' has an amount that appears to be your change from the transaction, shown in negative.

@gavinandresen

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2011

Nice catch on the listtransactions regression.

@luke-jr luke-jr referenced this pull request Dec 16, 2011

gavinandresen added some commits Sep 29, 2011

Support 3 new multisignature IsStandard transactions
Initial support for (a and b), (a or b), and 2-of-3 escrow
transactions (where a, b, and c are keys).
OP_EVAL implementation
OP_EVAL is a new opcode that evaluates an item on the stack as a script.
It enables a new type of bitcoin address that needs an arbitrarily
complex script to redeem.
Use block times for 'hard' OP_EVAL switchover, and refactored EvalScript
so it takes a flag for how to interpret OP_EVAL.
Also increased IsStandard size of scriptSigs to 500 bytes, so
a 3-of-3 multisig transaction IsStandard.

@gavinandresen gavinandresen merged commit 77f21f1 into bitcoin:master Dec 20, 2011

@gavinandresen

This comment has been minimized.

Copy link
Owner Author

commented on src/main.cpp in e679ec9 Dec 25, 2011

Should return error() if prevout.n >= txPrev.vout.size() (duplicate check in line 880)

ptschip pushed a commit to ptschip/bitcoin that referenced this pull request Jun 15, 2017

Merge pull request bitcoin#669 from kyuupichan/misbehaving
Move 3 members of CNodeState to CNode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.