Bitcoin Unlimited Code Review

[theZerg]

I would like an independent and preferably Bitcoin experienced developer to look at the changes I've made for BU. You can find the diffs here: https://github.com/gandrewstone/BitcoinUnlimited/commits/0.11cfg_stats

But if you do not have time to pore through the complete diffs I would really like validation on the following items:

1. Version

I know that there's some kind of spat between different version formats; I don't want to have to dig thru all that to understand what version is the best compromise... I just want to make sure BU signals BIP101 large blocks.

In CBlockHeader, I am setting the version to:
static const int32_t CURRENT_VERSION=0x40000007;

This is supposed to indicate both BIP101 and BIP65 support. Did I get that right?

2. Replaying transactions during chain reorganizations.

I switched from testnet <1MB to testnet >1MB chains which was a 1000+ fork reorg. This took forever and I realised that much of the time was in the transaction validation. But I reasoned that it should be unnecessary to re-validate these transactions because they were validated when the block was originally accepted. So this is my change, but I am uncertain about including it because I am not familiar with this area of the code and it is likely that the testnet monster chain reorg will not happen on the mainnet:

step 1: add a flag to AcceptToMemoryPool to indicate that this is a replayed transaction
bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransaction &tx, bool fLimitFree,
- bool* pfMissingInputs, bool fRejectAbsurdFee)
+ bool* pfMissingInputs, bool fRejectAbsurdFee, bool thisIsReplayed)

step 2: skip some validation if the transaction was already in a block

.
.
.
  if (!thisIsReplayed)  // BU: If this transaction is replayed (comes from a block we unwound) don't waste time reverifying it
  {
  // Continuously rate-limit free (really, very-low-fee) transactions
  // This mitigates 'penny-flooding' -- sending thousands of free transactions just to
  // be annoying or make others' transactions take longer to confirm.
  if (fLimitFree && nFees < ::minRelayTxFee.GetFee(nSize))
  {
  static CCriticalSection csFreeLimiter;
  static double dFreeCount;
  static int64_t nLastTime;
  int64_t nNow = GetTime();

  LOCK(csFreeLimiter);

  // Use an exponentially decaying ~10-minute window:
  dFreeCount *= pow(1.0 - 1.0/600.0, (double)(nNow - nLastTime));
  nLastTime = nNow;
  // -limitfreerelay unit is thousand-bytes-per-minute
  // At default rate it would take over a month to fill 1GB
  if (dFreeCount >= GetArg("-limitfreerelay", 15)*10*1000)
  return state.DoS(0, error("AcceptToMemoryPool: free transaction rejected by rate limiter"),
  REJECT_INSUFFICIENTFEE, "rate limited free transaction");
  LogPrint("mempool", "Rate limit dFreeCount: %g => %g\n", dFreeCount, dFreeCount+nSize);
  dFreeCount += nSize;
  }

  if (fRejectAbsurdFee && nFees > ::minRelayTxFee.GetFee(nSize) * 10000)
  return error("AcceptToMemoryPool: absurdly high fees %s, %d > %d",
  hash.ToString(),
  nFees, ::minRelayTxFee.GetFee(nSize) * 10000);

  // Check against previous transactions
  // This is done last to help prevent CPU exhaustion denial-of-service attacks.
  if (!CheckInputs(tx, state, view, true, STANDARD_SCRIPT_VERIFY_FLAGS, true))
  {
  return error("AcceptToMemoryPool: ConnectInputs failed %s", hash.ToString());
  }

  // Check again against just the consensus-critical mandatory script
  // verification flags, in case of bugs in the standard flags that cause
  // transactions to pass as valid when they're actually invalid. For
  // instance the STRICTENC flag was incorrectly allowing certain
  // CHECKSIG NOT scripts to pass, even though they were invalid.
  //
  // There is a similar check in CreateNewBlock() to prevent creating
  // invalid blocks, however allowing such transactions into the mempool
  // can be exploited as a DoS attack.
  if (!CheckInputs(tx, state, view, true, MANDATORY_SCRIPT_VERIFY_FLAGS, true))
  {
  return error("AcceptToMemoryPool: BUG! PLEASE REPORT THIS! ConnectInputs failed against MANDATORY but not STANDARD flags %s", hash.ToString());
  }
  }

step 3: In DisconnectTip, change AcceptToMemoryPool call to indicate that this is a txn replay:
if (tx.IsCoinBase() || !AcceptToMemoryPool(mempool, stateDummy, tx, false, NULL,false,true))
...

 

[solex]

@theZerg Great work. Impressive!

Got a few observations and questions (please bear with me as my c++ is very rusty).

Version number: I am wondering whether 0x20000008 is best as the existing BIP101 version already covers CLTV as this is just 4. Core did not attempt to use version bits for it (as they thought the BIP101 usage was imperfect and did not meet the emerging version bits usage specification). Once this specification is complete and has the necessary "consensus" then BU can be changed if necessary.

The old MAX_BLOCK_SIZE. Is this depreciated entirely except for determining the MAX_BLOCK_SIGOPS? If so, then perhaps it can be left at 1MB (politically best) or set to the BIP101 maximum of 8192MB?

The MAX_BLOCK_SIGOPS. Should this reflect the Core max sigops up to 1MB then, for any blocks accepted >1MB follow the BIP101 convention of sigops counting (@Gavin Andresen did significant improvements for this) . Otherwise, with the max sigops predicated on 32MB to start with, then is BU at risk from a "bloat" block attack?