Commentary

bitcoin · Jun 4, 2016 · 17389dc · 17389dc
1 parent fc772e4
commit 17389dc
Show file tree

Hide file tree

Showing 7 changed files with 58 additions and 0 deletions.
diff --git a/src/chainparams.cpp b/src/chainparams.cpp
@@ -241,6 +241,7 @@ static CTestNetParams testNetParams;
 /**
  * Segnet
  */
+// NOTE: segnet is not intended to be merged into Core. It was intended to allow testing before migrating to testnet.
 class CSegNetParams : public CChainParams {
 public:
     CSegNetParams() {

diff --git a/src/core_read.cpp b/src/core_read.cpp
@@ -90,6 +90,18 @@ CScript ParseScript(const std::string& s)
     return result;
 }
 
+/* NOTE: For every serialization corresponds to at most one valid transaction,
+         with or without witness. Thus, in every situation where we deal with
+         valid transactions, we can simply try to decode with witness flag
+         enabled, and there will be no ambiguity.
+
+         However, when dealing with potentially invalid transactions, things are
+         different. The decoderawtransaction and fundrawtransaction RPCs accept
+         serialized transaction that potentially don't have any inputs. Such a
+         serialization may be ambiguous. In those cases, and only those, we
+         first try to decode without, and then with, checking that the entire
+         serialization is consumed.
+*/
 bool DecodeHexTx(CTransaction& tx, const std::string& strHexTx, bool fTryNoWitness)
 {
     if (!IsHex(strHexTx))

diff --git a/src/main.cpp b/src/main.cpp
@@ -940,6 +940,10 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& in
     return nSigOps;
 }
 
+// NOTE: this is a tricky refactor. All call sites to GetLegacySigOpCount, GetP2SHSigOpCount, and
+//       the new CountWitnessSigOps are moved to this function, in both AcceptToMemoryPoolWorker
+//       and ConnectBlock. In ConnectBlock however, the existing calls are spread over a parent
+//       code block and a !tx.IsCoinBase() conditional (which is mimicked in this function),
 int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& inputs, int flags)
 {
     int64_t nSigOps = GetLegacySigOpCount(tx) * WITNESS_SCALE_FACTOR;
@@ -2412,6 +2416,10 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
             return state.DoS(100, error("ConnectBlock(): too many sigops"),
                              REJECT_INVALID, "bad-blk-sigops");
 
+        // NOTE: the existing (!tx.IsCoinBase()) conditional is split in two, as the above
+        // GetTransactionSigOpCost requires being called for non-coinbase transactions as
+        // well, but the HaveInputs() call above needs to be called before, and the
+        // script evaluation below needs to go after.
         if (!tx.IsCoinBase())
         {
             nFees += view.GetValueIn(tx)-tx.GetValueOut();
@@ -2675,6 +2683,11 @@ void static UpdateTip(CBlockIndex *pindexNew, const CChainParams& chainParams) {
 }
 
 /** Disconnect chainActive's tip. You probably want to call mempool.removeForReorg and manually re-limit mempool size after this, with cs_main held. */
+/* NOTE: Disconnecting many blocks is very slow if we want to retain mempool consistency.
+         During RewindBlock (called at startup, to go back to the state of the last block before the
+         fork), no entries exist in the mempool anyway, so we're able to skip all mempool-related
+         logic. The fBare parameter indicates that just chainstate but no mempool operations
+         need to be performed. */
 bool static DisconnectTip(CValidationState& state, const CChainParams& chainparams, bool fBare = false)
 {
     CBlockIndex *pindexDelete = chainActive.Tip();

diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
@@ -151,6 +151,10 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
     return true;
 }
 
+/* NOTE: For transactions we define a 'virtual transaction size' (equal to
+         transaction_cost / 4), which has no consensus meaning, but is used
+         for fee calculations, so there is a smooth transition from feerate
+         defined by size (for non-segwit transactions, size == vsize) */
 int64_t GetVirtualTransactionSize(const CTransaction& tx)
 {
     return (GetTransactionCost(tx) + WITNESS_SCALE_FACTOR - 1) / WITNESS_SCALE_FACTOR;

diff --git a/src/primitives/block.h b/src/primitives/block.h
@@ -38,6 +38,13 @@ class CBlockHeader
     template <typename Stream, typename Operation>
     inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {
         READWRITE(this->nVersion);
+        // NOTE: the assignment to the local variable nVersion is removed as it is unused. It was
+        //       intended to let serialization depend on block version number. That strategy would
+        //       lead to inconsistent behaviour, and you really need coordinated serialization changes
+        //       across the network instead. See the extended serialization format for transactions
+        //       instead, which is designed to be (1) backward compatible and (2) is purely a p2p
+        //       layer change that doesn't leak into consensus structures (so a new node can
+        //       downgrade when relaying to an old node, without breaking PoW).
         READWRITE(hashPrevBlock);
         READWRITE(hashMerkleRoot);
         READWRITE(nTime);

diff --git a/src/script/interpreter.h b/src/script/interpreter.h
@@ -156,6 +156,9 @@ class MutableTransactionSignatureChecker : public TransactionSignatureChecker
 bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& script, unsigned int flags, const BaseSignatureChecker& checker, SigVersion sigversion, ScriptError* error = NULL);
 bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, const CScriptWitness* witness, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* serror = NULL);
 
+/* NOTE: previously, sigop counting was done inside main, and implemented separately for P2SH and normal inputs.
+         This is not a very scalable approach if we expect more script improvements in the future that may
+         introduce new signature types. Thus, we move sigop counting to script, passing along the evaluation flags. */
 size_t CountWitnessSigOps(const CScript& scriptSig, const CScript& scriptPubKey, const CScriptWitness* witness, unsigned int flags);
 
 #endif // BITCOIN_SCRIPT_INTERPRETER_H
diff --git a/src/script/sign.cpp b/src/script/sign.cpp
@@ -286,6 +286,24 @@ static vector<valtype> CombineMultisig(const CScript& scriptPubKey, const BaseSi
 
 namespace
 {
+/* NOTE: a large amount of the code changes related to signing are due switching
+ *       internal functions to operate on byte arrays directly rather than
+ *       CScript encoded lists of pushes.
+ * 
+ *       However, that is (1) inconvenient in a segwit setting,
+ *       where we may need to convert the result of a normal signing operation
+ *       to a scriptwitness (which is not a script, but a vector of byte arrays,
+ *       see SignatureData) and (2) annoying even in the existing cases (for example,
+ *       you can't do manipulation like popping off a stack element without first
+ *       calling EvalScript).
+ *
+ *       So all internal signing code is changed to work on byte array vectors or the
+ *       Stacks type below, which is converted to a SignatureData type by all externally
+ *       callable functions. A side effect is that ProduceSignature can't write
+ *       directly into a CMutableTransaction anymore, but a separate UpdateTransaction
+ *       is used to convert the results in SignatureData to the scriptSig and scriptWitness
+ *       fields.
+ */
 struct Stacks
 {
     std::vector<valtype> script;