-
Notifications
You must be signed in to change notification settings - Fork 377
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
Revamp peg-in transactions #260
Revamp peg-in transactions #260
Conversation
I think I need to re-name "witness_program" and related fields to "witness_scriptpubkey". Program only refers to the hash of the witnessScript. |
a10fb10
to
840478f
Compare
split up history, should be compiling and passing tests at each commit. |
d8e92a1
to
a0850c7
Compare
removing WIP tag |
@@ -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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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.
return false; | ||
} | ||
if (tx.vin[i].m_is_pegin) { | ||
// This deals with p2sh in general only | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the trickiest part(versus previous implementation) is making sure that we are handling these cases correctly.
src/primitives/transaction.h
Outdated
@@ -413,6 +414,7 @@ class CTxIn | |||
CTxIn() | |||
{ | |||
nSequence = SEQUENCE_FINAL; | |||
m_is_pegin = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems redundant, since you already initialized it at the declaration. (in fact, nSequence
should probably also be initialized at declaration)
@@ -432,10 +434,10 @@ class CTxIn | |||
fHasAssetIssuance = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
} | ||
break; | ||
|
||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does +1 mean here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, just that I appreciate this code being removed..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from me too (for this whole file's diff)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my pleasure.
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/validation.cpp
Outdated
if (block.GetHash() == chainparams.GetConsensus().hashGenesisBlock) { | ||
if (!fJustCheck) { | ||
assert(block.vtx.size() == 1); | ||
assert(block.nHeight == 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the second assertion can stay.
src/validation.cpp
Outdated
|
||
CTxUndo undoDummy; | ||
UpdateCoins(tx, view, undoDummy, pindex->nHeight); | ||
const CTransaction tx = *(block.vtx[block.vtx.size()-1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/primitives/transaction.cpp
Outdated
@@ -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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we use the scriptWitness as the pegin_witness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoiding serializing two witness arrays which are never used simultaneously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But they are. During a peg-in both are used. pegin_witness shows what the "scriptPubKey" is, then scriptWitness satisfies it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, ok, I hadn't gotten that far yet. Never mind then :)
src/validation.cpp
Outdated
bool IsValidPeginWitness(const CScriptWitness& pegin_witness) { | ||
|
||
// Format on stack is as follows: | ||
// 1) txid - txid of parent chain transaction - TODO-PEGIN unneeded? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that computing the txid does not require understanding Bitcoin transactions. You just have to double-hash the serialized form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The txid is also already include in the prevout. We should drop it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as well as the nOut*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
src/validation.cpp
Outdated
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/validation.cpp
Outdated
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()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't prevout.n
have the pegin flag set here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the flag(as well as the issuance flag) is only set during serialization. During de-serialization it is masked out.
bac612c
to
f8d0af7
Compare
Updated PR addressing the major issues and cleanups. |
src/validation.cpp
Outdated
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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for both the prevout txid and the witness item. Great catch. Need to figure out the best way to handle these.
Updated with commit that deals with crash on "null" pegin. Left the asserts in place as they're useful for catching implementation errors. |
src/wallet/rpcwallet.cpp
Outdated
@@ -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 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the updated code seems to match, you're pointing at an old commit perhaps? Agreed on making it a utility function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sorry, you're commenting on an out of date comment in my code, not the code itself. Will fix this as well.
src/wallet/rpcwallet.cpp
Outdated
JSONRPCRequest request2; | ||
UniValue varr(UniValue::VARR); | ||
varr.push_back(strHex); | ||
request2.params = varr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
5733e82
to
3ff4aab
Compare
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)
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
This PR replaces the overly-complicated pegging methodology with something much simpler.
Old system:
getpeginaddress
stepNew system:
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 newpegin_witness
field that resides alongside thescriptWitness
field. It expects a certain number of elements on the stack. This includes: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:
Future Work: mininode support