Revamp peg-in transactions #260

instagibbs · 2017-09-14T18:09:58Z

This PR replaces the overly-complicated pegging methodology with something much simpler.

Old system:

21M bitcoin are locked with special templated script code called OP_WITHDRAWPROOFVERIFY
In order to peg-in the locked outputs are spent in special templated transactions
The pegged in funds must directly go to the address committed to in the getpeginaddress step
Inputs aren't signed at all. They are simply templated to only allow a single peg-in for the transaction, and no other inputs.
Peg-in outputs in sidechain must be the exact value of the claimed amount, and the rest must be locked up again.
Peg-ins cannot pay fees, accordingly.
Layer violations all over the place.

New system:

No templating, fewer layer violations, peg-ins can pay fees. Pegins are just "normal" spends of the corresponding Bitcoin output.

Details:
getpeginaddress now takes a key in its wallet, creates a witness program, makes SHA2 tweak using that program, then derives the address.

m_is_pegin in CTransaction tells the node to check its new pegin_witness field that resides alongside the scriptWitness field. It expects a certain number of elements on the stack. This includes:

// 1) value - the value of the pegin output (stored as special 8-byte CScriptNum)
// 2) asset type - the asset type being pegged in
// 3) genesis blockhash - genesis block of the parent chain
// 4) witness program - program that script evaluation will be evaluated against
// 5) serialized transaction - serialized bitcoin transaction
// 6) txout proof - merkle proof connecting transaction to header

The Bitcoin peg-in txid/nOut is included as the prevout, which makes sure the txid and signature covers the unique Bitcoin peg-in transaction. We can remove these to save a few bytes since they are completely redundant.

Once the node finds a properly formatted pegin witness, it uses the found witness program, and is directly evaluated as the inputs' scriptpubkey. SHA256(<witness program>) is also what the fedredeemscript keys are tweaked by.

The new witness is obviously malleable, which is why m_is_pegin is used to cover in signature. Care has to be taken that we aren't banning peers for malleated witnesses or including them into the mempool reject set.

Review action items:

Make sure the peg-in database is writing and re-writing which pegins have already occurred correctly.
Make sure peg-ins are being considered witness malleable and are treated kindly on the p2p layer
Understand the impact on watchman software

Future Work: mininode support

instagibbs · 2017-09-14T20:21:49Z

I think I need to re-name "witness_program" and related fields to "witness_scriptpubkey". Program only refers to the hash of the witnessScript.

instagibbs · 2017-09-15T19:26:52Z

split up history, should be compiling and passing tests at each commit.

instagibbs · 2017-09-18T17:50:16Z

removing WIP tag

arvidn · 2017-09-18T20:15:13Z

src/coins.cpp

@@ -330,6 +330,9 @@ bool CCoinsViewCache::HaveInputs(const CTransaction& tx) const
 {
    if (!tx.IsCoinBase()) {
        for (unsigned int i = 0; i < tx.vin.size(); i++) {
+            if (tx.vin[i].m_is_pegin) {


there's no risk with this? essentially to say that we "have" every peg-in input, regardless of if the peg-in is valid or not. (I assume "having" an input normally implies that it's valid/available)

"Have" usually means it exists in the UTXO database, but for peg-in inputs we aren't going to look there. Instead we are going to do two things:

Look for a well-formed pegin witness

Make sure that peg-in has not already occurred by looking for it in our "spent peg-in" database.

arvidn · 2017-09-18T20:50:03Z

src/policy/policy.cpp

-                        return false;
-            }
+        if (tx.vin[i].m_is_pegin) {
+            // This deals with p2sh in general only
            continue;


this is probably also something to look out for. non-peg-in inputs are validated to some degree, whereas peg-ins are not validated at all. It probably makes sense to ensure that no caller of this function relies all inputs being validated.

Yes, the trickiest part(versus previous implementation) is making sure that we are handling these cases correctly.

arvidn · 2017-09-18T20:53:56Z

src/primitives/transaction.h

@@ -413,6 +414,7 @@ class CTxIn
    CTxIn()
    {
        nSequence = SEQUENCE_FINAL;
+        m_is_pegin = false;


this seems redundant, since you already initialized it at the declaration. (in fact, nSequence should probably also be initialized at declaration)

arvidn · 2017-09-18T20:59:12Z

src/primitives/transaction.h

@@ -432,10 +434,10 @@ class CTxIn
                fHasAssetIssuance = false;


is it OK that prevout.n (and later outpoint.n) has the COutPoint::OUTPOINT_PEGIN_FLAG set here? even if m_is_pegin is false? Perhaps there's some invariant I'm not aware of, but if m_is_pegin is always set in this branch of the if, perhaps there should be an assert() to indicate that

this is a special casing for coinbase inputs. Can you point to the behavior you're worried about exactly? In both directions this case is handled.

I get the impression there's an invariant here along the lines of m_is_pegin <=> output.n & COutPoint::OUTPOINT_PEGIN_FLAG.

But in the (top) if-block (line 431 on the right), this invariant does not seem to be maintained, since prevout.n can have all bits set, regardless of what m_is_pegin is. i.e., that branch of the if-statement allows serializing the OUTPOINT_PEGIN_FLAG regardless.

Perhaps m_is_pegin is always true if prevout.n == -1. If so, perhaps it would make sense to add an assert to indicate that.

Right, it's another magic value for coinbase input values, which also effects issuance bit logic.

Perhaps more rigorous asserts are needed; any specific places to suggest?

arvidn · 2017-09-18T21:04:54Z

src/script/interpreter.cpp

-                }
-                break;
-
-                default:


what does +1 mean here

oh, just that I appreciate this code being removed..

+1 from me too (for this whole file's diff)

my pleasure.

arvidn · 2017-09-18T21:11:35Z

src/script/standard.h

@@ -40,7 +40,7 @@ extern unsigned nMaxDatacarrierBytes;
 * Failing one of these tests may trigger a DoS ban - see CheckInputs() for
 * details.
 */
-static const unsigned int MANDATORY_SCRIPT_VERIFY_FLAGS = SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY | SCRIPT_VERIFY_WITHDRAW;


I believe you can remove this constant (SCRIPT_VERIFY_WITHDRAW) all together: https://github.com/ElementsProject/elements/blob/elements-0.14.1/src/script/interpreter.h#L113

apoelstra · 2017-09-20T14:43:26Z

src/validation.cpp

    if (block.GetHash() == chainparams.GetConsensus().hashGenesisBlock) {
        if (!fJustCheck) {
-            assert(block.vtx.size() == 1);
-            assert(block.nHeight == 0);


I think the second assertion can stay.

apoelstra · 2017-09-20T14:45:35Z

src/validation.cpp


-            CTxUndo undoDummy;
-            UpdateCoins(tx, view, undoDummy, pindex->nHeight);
+            const CTransaction tx = *(block.vtx[block.vtx.size()-1]);


Should this be a loop over all coinbase transactions? If we ever have a situation where AppendPolicyIssuance is called more than once, this code means that only the last one's outputs are spendable.

apoelstra · 2017-09-20T15:30:13Z

src/primitives/transaction.cpp

@@ -120,6 +121,7 @@ uint256 CTxInWitness::GetHash() const
    leaves.push_back(SerializeHash(vchIssuanceAmountRangeproof, SER_GETHASH, 0));
    leaves.push_back(SerializeHash(vchInflationKeysRangeproof, SER_GETHASH, 0));
    leaves.push_back(SerializeHash(scriptWitness.stack, SER_GETHASH, 0));
+    leaves.push_back(SerializeHash(pegin_witness.stack, SER_GETHASH, 0));


Why can't we use the scriptWitness as the pegin_witness?

what's the advantage of doing so? Seems to be abuse, since they serve two separate roles:
scriptWitness: utxo spending authorization
pegin_witness: peg-in authorization

Avoiding serializing two witness arrays which are never used simultaneously.

But they are. During a peg-in both are used. pegin_witness shows what the "scriptPubKey" is, then scriptWitness satisfies it.

Oh, ok, I hadn't gotten that far yet. Never mind then :)

apoelstra · 2017-09-20T15:39:43Z

src/validation.cpp

+bool IsValidPeginWitness(const CScriptWitness& pegin_witness) {
+
+    // Format on stack is as follows:
+    // 1) txid - txid of parent chain transaction - TODO-PEGIN unneeded?


I think we should drop the txid, it's cheap to compute and it's an extra thing to validate. Especially now that the transaction is serialized entirely without that 520-byte splitting frustration.

Note that computing the txid does not require understanding Bitcoin transactions. You just have to double-hash the serialized form.

The txid is also already include in the prevout. We should drop it.

as well as the nOut*

apoelstra · 2017-09-20T15:54:03Z

src/validation.cpp

+    // Check that the witness program matches the p2ch on the transaction output
+    opcodetype opcodeTmp;
+    unsigned char tweak[32];
+    CSHA256().Write(witness_program.data(), witness_program.size()).Finalize(tweak);


The tweak should somehow include the fedpegScript. In the original scheme each pubkey goes into the hash that computes its tweak, which is the standard way to do pay-to-contract and is always secure.

As written, it is possible in principle for the functionaries to adversarially choose their keys to steal some existing Bitcoin output. (They'd be unable to process any other peg-outs, and the existing output would need to be in the correct multisig form, so this is a silly attack scenario, but it is there.)

So there are two options: (1) include fedpegScript in its entirety in the tweak, or (2) in the key-tweaking loop recompute the tweak each time using the standard pay-to-contract scheme.

apoelstra · 2017-09-20T16:50:41Z

src/validation.cpp

-            assert(coins);
+            if (tx.vin[i].m_is_pegin) {
+                // Check existence and validity of pegin witness
+                if (tx.wit.vtxinwit.size() <= i || !IsValidPeginWitness(tx.wit.vtxinwit[i].pegin_witness) || prevout.hash != uint256(tx.wit.vtxinwit[i].pegin_witness.stack[0]) || (int)prevout.n != CScriptNum(tx.wit.vtxinwit[i].pegin_witness.stack[1], true).getint()) {


Won't prevout.n have the pegin flag set here?

the flag(as well as the issuance flag) is only set during serialization. During de-serialization it is masked out.

instagibbs · 2017-09-25T16:51:00Z

Updated PR addressing the major issues and cleanups.

jonasnick · 2017-09-26T14:38:18Z

src/validation.cpp

+                    return state.Invalid(false, REJECT_INVALID, "pegin-no-witness");
+                }
+                std::pair<uint256, COutPoint> pegin = std::make_pair(uint256(tx.wit.vtxinwit[i].pegin_witness.stack[2]), tx.vin[i].prevout);
+                if (view.IsWithdrawSpent(pegin)) {


Could a maliciously crafted pegin with uint256(tx.wit.vtxinwit[i].pegin_witness.stack[2]).IsNull() == true crash the node by triggering the assert in IsWithdrawSpent?

Yes, for both the prevout txid and the witness item. Great catch. Need to figure out the best way to handle these.

instagibbs · 2017-09-26T15:48:28Z

Updated with commit that deals with crash on "null" pegin. Left the asserts in place as they're useful for catching implementation errors.

apoelstra · 2017-10-02T18:03:37Z

src/wallet/rpcwallet.cpp

@@ -3388,6 +3389,368 @@ class CSecp256k1Init {
 static CSecp256k1Init instance_of_csecp256k1;
 }

+/* Takes federation redeeem script and adds SHA2(witnessProgram) as a tweak to each pubkey */


Recommend moving this out of rpcwallet.cpp and making it a general utility function which can also be used in consensus.

Right now this function looks correct but its mirror code (see my comment on 65158b7) in peg-in validation is wrong.

BTW is there a test that catches this discrepancy?

the updated code seems to match, you're pointing at an old commit perhaps? Agreed on making it a utility function.

oh sorry, you're commenting on an out of date comment in my code, not the code itself. Will fix this as well.

apoelstra · 2017-10-02T18:29:32Z

src/wallet/rpcwallet.cpp

+    JSONRPCRequest request2;
+    UniValue varr(UniValue::VARR);
+    varr.push_back(strHex);
+    request2.params = varr;


Can we split everything up to here into a createrawpegin RPC, since it looks like the standard sendrawtransaction and signrawtransaction will work to sign and send it? We can leave claimpegin which does all three as a convenience method. But as an advanced user it make me nervous when Core produces transactions then signs and publishes them without my review.

instagibbs · 2017-11-06T16:04:27Z

did history janitor work, will stop messing with it.

3ff4aab update unit test (Gregory Sanders) 6cc380a don't require claim_script to be witness program for raw claim pegin (Gregory Sanders) da6960d validation variables changed to not assume witness program for pegin (Gregory Sanders) 2940c4e remove tabs from IsValidPegin (Gregory Sanders) b401ff0 change explanation of policy asset seeding in python tutorial (Gregory Sanders) 7dc4f9d remove CNumScript from pegin witness logic (Gregory Sanders) abafbac change rpc test witness_program to claim_script (Gregory Sanders) 851a30b Don't restrict pegin witness to 'witness program' definition (Gregory Sanders) 13c3dad de-dupe calculate_contract code (Gregory Sanders) 6534bd3 Add is_pegin response for raw transaction RPCs (Gregory Sanders) d4d00da basic functional test for peg-in reorgs in sidechain (Gregory Sanders) 4aad513 Add unit tests for peg-in authorization (Gregory Sanders) be735cf re-add pegging python test (Gregory Sanders) 3ba146d update assets tutorial to support new peg-in format (Gregory Sanders) 6e8e6b2 add wallet support for pegging functionality (Gregory Sanders) 7633839 add raw rpc support for peg-in functionality (Gregory Sanders) ff5e9cd mempool and policy support for peg-in inputs (Gregory Sanders) 6bad4e9 add blocks that have pegins rejected to the reconsider queue (Gregory Sanders) 0b9c5eb begin consensus support for peg-ins (Gregory Sanders) 34d7cd0 add pegin witness validation and extraction helper functions (Gregory Sanders) 16973cd add pegin_witness to witness inputs (Gregory Sanders) 36b3ab1 add serialization flag support for peg-in inputs (Gregory Sanders) 9a9ac25 genesis block has actual fee issuance in regtest (Gregory Sanders) 65ed777 remove legacy peg-in/withdrawlocks (Gregory Sanders)

instagibbs · 2017-11-07T18:03:48Z

Merged.

8591de9 kill useless mapPeginsSpentToTxid (Gregory Sanders) Pull request description: Completely redundant since #260 when peg-in inputs have taken on deterministic prevouts, so normal mempool logic has taken over for accounting for double-spending peg-in proofs in the mempool. resolves #624 Tree-SHA512: e5c0e7570657bd1418f54fdd47a6f3f815a5779e28ff15ba37c18e425780415385af64efa57be882ba12ad748e7a664fb723eae0e819bf1d5f197bb0b82b086b

instagibbs changed the title ~~Revemp peg-in transactions~~ [WIP] Revemp peg-in transactions Sep 14, 2017

instagibbs force-pushed the unsuck_peg_squashed branch 2 times, most recently from a10fb10 to 840478f Compare September 15, 2017 19:26

instagibbs changed the title ~~[WIP] Revemp peg-in transactions~~ [WIP] Revamp peg-in transactions Sep 18, 2017

instagibbs force-pushed the unsuck_peg_squashed branch from d8e92a1 to a0850c7 Compare September 18, 2017 17:49

instagibbs changed the title ~~[WIP] Revamp peg-in transactions~~ Revamp peg-in transactions Sep 18, 2017

arvidn reviewed Sep 18, 2017

View reviewed changes

apoelstra reviewed Sep 20, 2017

View reviewed changes

jtimon mentioned this pull request Sep 21, 2017

Refactor: Easier support for chains with multiple genesis assets #191

Closed

3 tasks

instagibbs mentioned this pull request Sep 21, 2017

simplify peg-in process #254

Closed

instagibbs force-pushed the unsuck_peg_squashed branch 2 times, most recently from bac612c to f8d0af7 Compare September 25, 2017 16:50

jonasnick reviewed Sep 26, 2017

View reviewed changes

apoelstra reviewed Oct 2, 2017

View reviewed changes

instagibbs added 20 commits November 6, 2017 10:13

add pegin witness validation and extraction helper functions

34d7cd0

begin consensus support for peg-ins

0b9c5eb

add blocks that have pegins rejected to the reconsider queue

6bad4e9

mempool and policy support for peg-in inputs

ff5e9cd

add raw rpc support for peg-in functionality

7633839

add wallet support for pegging functionality

6e8e6b2

update assets tutorial to support new peg-in format

3ba146d

re-add pegging python test

be735cf

Add unit tests for peg-in authorization

4aad513

basic functional test for peg-in reorgs in sidechain

d4d00da

Add is_pegin response for raw transaction RPCs

6534bd3

de-dupe calculate_contract code

13c3dad

Don't restrict pegin witness to 'witness program' definition

851a30b

change rpc test witness_program to claim_script

abafbac

remove CNumScript from pegin witness logic

7dc4f9d

change explanation of policy asset seeding in python tutorial

b401ff0

remove tabs from IsValidPegin

2940c4e

validation variables changed to not assume witness program for pegin

da6960d

don't require claim_script to be witness program for raw claim pegin

6cc380a

update unit test

3ff4aab

instagibbs force-pushed the unsuck_peg_squashed branch from 5733e82 to 3ff4aab Compare November 6, 2017 16:03

instagibbs merged commit 3ff4aab into ElementsProject:elements-0.14.1 Nov 7, 2017

instagibbs mentioned this pull request Nov 8, 2017

update api to cover peg-in revamp ElementsProject/elementsbp-api-reference#5

Open

instagibbs mentioned this pull request Sep 28, 2018

peg-in witness invalid blocks disk at on start cause log spam/DoS #424

Closed

instagibbs mentioned this pull request May 20, 2019

kill useless mapPeginsSpentToTxid #627

Merged

gwillen pushed a commit that referenced this pull request Jun 1, 2022

Merge 03eccee into merged_master (Bitcoin PR #260)

21e1034

Revamp peg-in transactions #260

Revamp peg-in transactions #260

Conversation

instagibbs commented Sep 14, 2017 • edited

instagibbs commented Sep 14, 2017

instagibbs commented Sep 15, 2017

instagibbs commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvidn Sep 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Sep 25, 2017

jonasnick Sep 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Nov 6, 2017

instagibbs commented Nov 7, 2017

instagibbs commented Sep 14, 2017 •

edited

arvidn Sep 18, 2017 •

edited

jonasnick Sep 26, 2017 •

edited