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

Rewrite DoS interface between validation and net_processing #15141

Merged
merged 20 commits into from May 4, 2019
Merged
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
8818729
[refactor] Refactor misbehavior ban decisions to MaybePunishNode()
TheBlueMatt Apr 16, 2018
b8b4c80
[refactor] drop IsInvalid(nDoSOut)
TheBlueMatt Jan 17, 2019
f34fa71
Drop obsolete sigops comment
TheBlueMatt Jan 18, 2019
00e11e6
[refactor] rename stateDummy -> orphan_state
TheBlueMatt Jan 22, 2019
7b99910
Clean up banning levels
TheBlueMatt Jan 23, 2019
6a7f877
Ban all peers for all block script failures
sdaftuar Apr 16, 2019
34477cc
[refactor] Add useful-for-dos "reason" field to CValidationState
TheBlueMatt Jan 16, 2019
c8b0d22
[refactor] Drop redundant nDoS, corruptionPossible, SetCorruptionPoss…
TheBlueMatt Jan 18, 2019
7df16e7
LookupBlockIndex -> CACHED_INVALID
TheBlueMatt Jan 17, 2019
6e55b29
CorruptionPossible -> TX_WITNESS_MUTATED
TheBlueMatt Jan 17, 2019
9ab2a04
CorruptionPossible -> BLOCK_MUTATED
TheBlueMatt Jan 18, 2019
ef54b48
[refactor] Use Reasons directly instead of DoS codes
TheBlueMatt Nov 8, 2017
6b34bc6
Fix handling of invalid headers
sdaftuar Jan 15, 2019
5e78c57
Allow use of state.Invalid() for all reasons
TheBlueMatt Jan 20, 2019
7721ad6
[refactor] Prep for scripted-diff by removing some \ns which annoy sed.
TheBlueMatt Nov 8, 2017
aa502b8
scripted-diff: Remove DoS calls to CValidationState
TheBlueMatt Apr 11, 2019
12dbdd7
[refactor] Drop unused state.DoS(), state.GetDoS(), state.CorruptionP…
TheBlueMatt Jan 18, 2019
2120c31
[refactor] Update some comments in validation.cpp as we arent doing D…
TheBlueMatt Apr 17, 2018
54470e7
Assert validation reasons are contextually correct
sdaftuar Feb 21, 2019
0ff1c2a
Separate reason for premature spends (coinbase/locktime)
sdaftuar Mar 8, 2019
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

[refactor] Add useful-for-dos "reason" field to CValidationState

This is a first step towards cleaning up our DoS interface - make
validation return *why* something is invalid, and let net_processing
figure out what that implies in terms of banning/disconnection/etc.

Behavior change: peers will now be banned for providing blocks
with premature coinbase spends.

Co-authored-by: Anthony Towns <aj@erisian.com.au>
                Suhas Daftuar <sdaftuar@gmail.com>
  • Loading branch information
TheBlueMatt and sdaftuar committed Jan 16, 2019
commit 34477ccd39a8d4bfa8ad612f22d5a46291922185
@@ -11,24 +11,24 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe
{
// Basic checks that don't depend on any context
if (tx.vin.empty())
return state.DoS(100, false, REJECT_INVALID, "bad-txns-vin-empty");
return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vin-empty");

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 30, 2019

Member

There's a very large correlation with the reject codes here e.g. ValidationInvalidReason::CONSENSUS<->REJECT_INVALID. Not really a problem, though adding another way to classify every instance of Invalid/DoS seems a bit overkill and the number of parameters keeps growing.

Edit: couldn't we replace the reject code with a mapping from ValidationInvalidReason?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 30, 2019

Member

See commit 903d68f in follow-up PR #15921 which removes the reject code from ValidationState.

if (tx.vout.empty())
return state.DoS(100, false, REJECT_INVALID, "bad-txns-vout-empty");
return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vout-empty");
// Size limits (this doesn't take the witness into account, as that hasn't been checked for malleability)
if (::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT)
return state.DoS(100, false, REJECT_INVALID, "bad-txns-oversize");
return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-oversize");

// Check for negative or overflow output values
CAmount nValueOut = 0;
for (const auto& txout : tx.vout)
{
if (txout.nValue < 0)
return state.DoS(100, false, REJECT_INVALID, "bad-txns-vout-negative");
return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vout-negative");
if (txout.nValue > MAX_MONEY)
return state.DoS(100, false, REJECT_INVALID, "bad-txns-vout-toolarge");
return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vout-toolarge");
nValueOut += txout.nValue;
if (!MoneyRange(nValueOut))
return state.DoS(100, false, REJECT_INVALID, "bad-txns-txouttotal-toolarge");
return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-txouttotal-toolarge");
}

// Check for duplicate inputs - note that this check is slow so we skip it in CheckBlock
@@ -37,20 +37,20 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe
for (const auto& txin : tx.vin)
{
if (!vInOutPoints.insert(txin.prevout).second)
return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputs-duplicate");
return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-inputs-duplicate");
}
}

if (tx.IsCoinBase())
{
if (tx.vin[0].scriptSig.size() < 2 || tx.vin[0].scriptSig.size() > 100)
return state.DoS(100, false, REJECT_INVALID, "bad-cb-length");
return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-cb-length");
}
else
{
for (const auto& txin : tx.vin)
if (txin.prevout.IsNull())
return state.DoS(100, false, REJECT_INVALID, "bad-txns-prevout-null");
return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-prevout-null");
}

return true;
@@ -160,7 +160,7 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, c
{
// are the actual inputs available?
if (!inputs.HaveInputs(tx)) {
return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputs-missingorspent", false,
return state.DoS(0, ValidationInvalidReason::TX_MISSING_INPUTS, false, REJECT_INVALID, "bad-txns-inputs-missingorspent", false,
strprintf("%s: inputs missing/spent", __func__));
}

@@ -172,28 +172,28 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, c

// If prev is coinbase, check that it's matured
if (coin.IsCoinBase() && nSpendHeight - coin.nHeight < COINBASE_MATURITY) {
return state.DoS(0, false,
return state.DoS(0, ValidationInvalidReason::TX_MISSING_INPUTS, false,
REJECT_INVALID, "bad-txns-premature-spend-of-coinbase", false,

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 11, 2019

Contributor

In commit "Clean up banning levels" (96cedc8)

Note: this is "Inclusion of a premature coinbase spend now results in a ban"

strprintf("tried to spend coinbase at depth %d", nSpendHeight - coin.nHeight));
}

// Check for negative or overflow input values
nValueIn += coin.out.nValue;
if (!MoneyRange(coin.out.nValue) || !MoneyRange(nValueIn)) {
return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputvalues-outofrange");
return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-inputvalues-outofrange");
}
}

const CAmount value_out = tx.GetValueOut();
if (nValueIn < value_out) {
return state.DoS(100, false, REJECT_INVALID, "bad-txns-in-belowout", false,
return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-in-belowout", false,
strprintf("value in (%s) < value out (%s)", FormatMoney(nValueIn), FormatMoney(value_out)));
}

// Tally transaction fees
const CAmount txfee_aux = nValueIn - value_out;
if (!MoneyRange(txfee_aux)) {
return state.DoS(100, false, REJECT_INVALID, "bad-txns-fee-outofrange");
return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-fee-outofrange");
}

txfee = txfee_aux;
@@ -22,6 +22,50 @@ static const unsigned char REJECT_NONSTANDARD = 0x40;
static const unsigned char REJECT_INSUFFICIENTFEE = 0x42;
static const unsigned char REJECT_CHECKPOINT = 0x43;

/** A "reason" why something was invalid, suitable for determining whether the
* provider of the object should be banned/ignored/disconnected/etc.
* These are much more granular than the rejection codes, which may be more
* useful for some other use-cases.
*/
enum class ValidationInvalidReason {
// txn and blocks:
NONE, //!< not actually invalid

This comment has been minimized.

Copy link
@kallewoof

kallewoof Mar 5, 2019

Member

I understand if you don't wanna bother this late in the process, but it really seems like ValidationResult or ValidationOutcome would have been easier on the eyes, where this would be SUCCESSFUL or something.

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Mar 7, 2019

Author Member

I'd prefer to rename this in a future PR, since that would be a simple change and I don't want to hold up the structural improvements here.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 16, 2019

Member

nit: This should be "not invalid or not yet tested for validity"

CONSENSUS, //!< invalid by consensus rules (excluding any below reasons)
/**
* Invalid by a change to consensus rules more recent than SegWit.
* Currently unused as there are no such consensus rule changes, and any download
* sources realistically need to support SegWit in order to provide useful data,
* so differentiating between always-invalid and invalid-by-pre-SegWit-soft-fork
* is uninteresting.
*/
RECENT_CONSENSUS_CHANGE,

This comment has been minimized.

Copy link
@jamesob

jamesob Apr 2, 2019

Member

This seems fluffy - I suspect we'd want to call out specific changes if we were to actually do something like this.
I guess there's value in having it here as a conceptual placeholder, but I'd be surprised if this symbol ever actually got used.

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Apr 3, 2019

Author Member

Happy to remove if others agree since that was my preference as well, but I left this in because Matt preferred it when we originally discussed this on his PR.

This comment has been minimized.

Copy link
@jamesob

jamesob Apr 3, 2019

Member

It's not hurting anything; I'd rather have this merged and remove it later.

// Only blocks (or headers):
CACHED_INVALID, //!< this object was cached as being invalid, but we don't know why

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 16, 2019

Member

nit: change "we don't know why" to "we didn't store the reason why"

BLOCK_INVALID_HEADER, //!< invalid proof of work or time too old
BLOCK_MUTATED, //!< the block's data didn't match the data committed to by the PoW
BLOCK_MISSING_PREV, //!< We don't have the previous block the checked one is built on
BLOCK_INVALID_PREV, //!< A block this one builds on is invalid
BLOCK_TIME_FUTURE, //!< block timestamp was > 2 hours in the future (or our clock is bad)
BLOCK_CHECKPOINT, //!< the block failed to meet one of our checkpoints
// Only loose txn:
TX_NOT_STANDARD, //!< didn't meet our local policy rules
TX_MISSING_INPUTS, //!< a transaction was missing some of its inputs (or its inputs were spent at < coinbase maturity height)
/**
* Transaction might be missing a witness, have a witness prior to SegWit
* activation, or witness may have been malleated (which includes
* non-standard witnesses).
*/
TX_WITNESS_MUTATED,
/**
* Tx already in mempool or conflicts with a tx in the chain
This conversation was marked as resolved by sdaftuar

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jan 14, 2019

Member

nit: s/conflicts with a tx in the chain/conflicts with a confirmed transaction. Same comment below for "exists in the mempool or on chain"

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jan 31, 2019

Author Member

I don't really think "confirmed transaction" is any clearer than "tx in the chain" -- if anything, the latter seems more specific to me, as "confirmed" is a concept that only makes sense in the context of the chain that you're on, which "tx in the chain" is more explicit about.

I'm going to leave this comment intact, pending other opinions.

This comment has been minimized.

Copy link
@sipa

sipa Feb 8, 2019

Member

I think the current wording is fine.

* (if it conflicts with another tx in mempool, we use MEMPOOL_POLICY as it failed to reach the RBF threshold)
* TODO: Currently this is only used if the transaction already exists in the mempool or on chain,
* TODO: ATMP's fMissingInputs and a valid CValidationState being used to indicate missing inputs

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 16, 2019

Member

nit: I think this TODO should be next to the TX_MISSING_INPUTS line

*/
TX_CONFLICT,
TX_MEMPOOL_POLICY, //!< violated mempool's fee/size/descendant/RBF/etc limits
};

/** Capture information about block/transaction validation */
class CValidationState {
private:
@@ -30,31 +74,35 @@ class CValidationState {
MODE_INVALID, //!< network rule violation (DoS value may be set)
MODE_ERROR, //!< run-time error
} mode;
ValidationInvalidReason m_reason;
This conversation was marked as resolved by sdaftuar

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Apr 9, 2019

Contributor

Can you squash "nit: reason -> m_reason"? Please don't add full commits that just address nits. Ignoring the nit is also perfectly valid.

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Apr 11, 2019

Author Member

Fixed.

int nDoS;
std::string strRejectReason;
unsigned int chRejectCode;
bool corruptionPossible;
std::string strDebugMessage;
public:
CValidationState() : mode(MODE_VALID), nDoS(0), chRejectCode(0), corruptionPossible(false) {}
bool DoS(int level, bool ret = false,
CValidationState() : mode(MODE_VALID), m_reason(ValidationInvalidReason::NONE), nDoS(0), chRejectCode(0), corruptionPossible(false) {}
bool DoS(int level, ValidationInvalidReason reasonIn, bool ret = false,
unsigned int chRejectCodeIn=0, const std::string &strRejectReasonIn="",
bool corruptionIn=false,
const std::string &strDebugMessageIn="") {
m_reason = reasonIn;
chRejectCode = chRejectCodeIn;
strRejectReason = strRejectReasonIn;
corruptionPossible = corruptionIn;
strDebugMessage = strDebugMessageIn;
nDoS += level;
assert(nDoS == GetDoSForReason());
assert(corruptionPossible == (m_reason == ValidationInvalidReason::BLOCK_MUTATED || m_reason == ValidationInvalidReason::TX_WITNESS_MUTATED));
if (mode == MODE_ERROR)
return ret;
nDoS += level;

This comment has been minimized.

Copy link
@Sjors

Sjors Mar 6, 2019

Member

What was the original idea behind bumping this? Was it to continue validating and perhaps find additional errors before giving up, to see if the originating peer needs stronger punishment?

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Mar 7, 2019

Author Member

I don't know the history here, but that seems like a reasonable guess.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Apr 9, 2019

Contributor

Can you just drop this commit? Its really nontrivial to review and, by the end, is unused anyway.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 11, 2019

Member

re: #15141 (comment)

This needs to be done in this early commit so that the second half of this PR is refactor-only. Once we move to invalid-reason based punishment, calling Invalid() on a CValidationState object sets the reason and doesn't 'increment' it.

I think this commit should stay, possible squashed with the following commit (Clean up banning levels) but with an expanded git commit log explaining why the change is being made and highlighting what reviewers should check to satisfy themselves that this isn't a behaviour change (that DoS() is never called more than once on the same object).

mode = MODE_INVALID;
return ret;
}
bool Invalid(bool ret = false,
bool Invalid(ValidationInvalidReason _reason, bool ret = false,
unsigned int _chRejectCode=0, const std::string &_strRejectReason="",
const std::string &_strDebugMessage="") {
return DoS(0, ret, _chRejectCode, _strRejectReason, false, _strDebugMessage);
return DoS(0, _reason, ret, _chRejectCode, _strRejectReason, false, _strDebugMessage);
}
bool Error(const std::string& strRejectReasonIn) {
if (mode == MODE_VALID)
@@ -72,12 +120,39 @@ class CValidationState {
return mode == MODE_ERROR;
}
bool CorruptionPossible() const {
assert(corruptionPossible == (m_reason == ValidationInvalidReason::BLOCK_MUTATED || m_reason == ValidationInvalidReason::TX_WITNESS_MUTATED));
return corruptionPossible;
}

This comment has been minimized.

Copy link
@ajtowns

ajtowns Jan 15, 2019

Contributor

Maybe having inline bool CorruptionPossible() const { return reason == BLOCK_MUTATED; } would make for nicer code elsewhere?

void SetCorruptionPossible() {
corruptionPossible = true;
assert(corruptionPossible == (m_reason == ValidationInvalidReason::BLOCK_MUTATED || m_reason == ValidationInvalidReason::TX_WITNESS_MUTATED));
}
int GetDoS(void) const { return nDoS; }
int GetDoSForReason() const {

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Apr 9, 2019

Contributor

This is nonsense, why is it in consensus code, and why is it removed later? It just makes review harder.

switch (m_reason) {
case ValidationInvalidReason::NONE:
return 0;
case ValidationInvalidReason::CONSENSUS:
case ValidationInvalidReason::BLOCK_MUTATED:
case ValidationInvalidReason::BLOCK_INVALID_HEADER:
case ValidationInvalidReason::BLOCK_CHECKPOINT:
case ValidationInvalidReason::BLOCK_INVALID_PREV:
return 100;
case ValidationInvalidReason::BLOCK_MISSING_PREV:
return 10;
case ValidationInvalidReason::CACHED_INVALID:
case ValidationInvalidReason::RECENT_CONSENSUS_CHANGE:
case ValidationInvalidReason::BLOCK_TIME_FUTURE:
case ValidationInvalidReason::TX_NOT_STANDARD:
case ValidationInvalidReason::TX_MISSING_INPUTS:

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 11, 2019

Contributor

In commit "TX_MISSING_INPUTS now has a DoS score of 0" (bee1d4f)

Not sure, but it seems like it might be nice to have a comment here saying TX_MISSING_INPUTS reason will change to CONSENSUS if the transaction is included in a block, and lead to a higher dos score in that case.

case ValidationInvalidReason::TX_WITNESS_MUTATED:
case ValidationInvalidReason::TX_CONFLICT:
case ValidationInvalidReason::TX_MEMPOOL_POLICY:
return 0;
}
return 0;
}
ValidationInvalidReason GetReason() const { return m_reason; }
unsigned int GetRejectCode() const { return chRejectCode; }
std::string GetRejectReason() const { return strRejectReason; }
std::string GetDebugMessage() const { return strDebugMessage; }
@@ -961,6 +961,7 @@ void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIV

static bool TxRelayMayResultInDisconnect(const CValidationState& state)
{
assert(state.GetDoS() == state.GetDoSForReason());
return (state.GetDoS() > 0);
}

@@ -975,6 +976,7 @@ static bool TxRelayMayResultInDisconnect(const CValidationState& state)
* txs, the peer should not be punished. See BIP 152.
*/
static bool MaybePunishNode(NodeId nodeid, const CValidationState& state, bool via_compact_block, const std::string& message = "") {
This conversation was marked as resolved by sdaftuar

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Apr 9, 2019

Contributor

"Use maybepunish" needs a real commitmessage. Wtf is a "maybepunish" and why are we using it?

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Apr 11, 2019

Author Member

Done in latest version.

assert(state.GetDoS() == state.GetDoSForReason());
int nDoS = state.GetDoS();
if (nDoS > 0 && !via_compact_block) {
LOCK(cs_main);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 24, 2019

Contributor

In commit "[refactor] Use maybepunish etc" (8226bed)

Note: This acquires lock recursively in PeerLogicValidation::BlockChecked. Seems fine, but just wanted to note it wasn't happening before.

@@ -52,6 +52,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup)
// Check that the validation state reflects the unsuccessful attempt.
BOOST_CHECK(state.IsInvalid());
BOOST_CHECK_EQUAL(state.GetRejectReason(), "coinbase");
BOOST_CHECK(state.GetReason() == ValidationInvalidReason::CONSENSUS);
}

BOOST_AUTO_TEST_SUITE_END()
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.