Anti Header Spam v2

[aguycalled]

This Pull Request proposes a new anti header spam system which aims to fix some of the issues of the previous implementation (including those reported by art-of-bug).

Features:

• Every time a header or block is received from another peer, its hash is added to a points list associated with the peer.
• Peers are discerned by their ip address, this means peers sharing ip address will also share the same points list. This can be changed with -headerspamfilterignoreport (default: true).
• Before proceeding with the block or headers validation, the points list will be cleared removing all the hashes of blocks whose scripts have already been correctly validated.
• The peer is banned if the size of the points list is greater than MAX_HEADERS_RESULTS*2 once cleared of already validated blocks.
• The maximum allowed size of the points list can be changed using the -headerspamfiltermaxsize parameter.
• The log category headerspam has been added, which prints to the log the current size of a peers points list.
• When -debug=bench is specified, execution time for the updateState function is logged.

Considerations

• The maximum size of the points list by default is 4,000. With a block time of 30 seconds, NavCoin sees an average of 2,880 blocks per day. A maximum value of 4000 is roughly one and a half times more than the count of blocks a peer needs to be behind the chain tip to be in Initial Block Download mode. When on IBD, the header spam filter is turned off. This ensures that normal synchronisation is not affected by this filter.
• An attacker would be able to exhaust 32 bytes from the hash inserted in the points list + 181 bytes from the CBlockIndex inserted in mapBlockIndex for every invalid header/block before being banned. The points list is cleared when the attacker is banned, but those headers are not removed from mapBlockIndex or the hard disk in the current implementation. The size of CBlockIndex has been measured with:

    CBlockIndex* pindex = new CBlockIndex();
    CDataStream ssPeers(SER_DISK, CLIENT_VERSION);
    ss << CDiskBlockIndex(pindex);
    std::vector<unsigned char>vch(ss.begin(), ss.end());
    std::cout << to_string(vch.size()) << std::endl;

• The default maximum value means that a single malicious peer with a unique IP can exhaust at max 3,999*213=831 kilobytes without being banned or 4,000*181=707 kilobytes being banned.

What to test

• Legit nodes are not banned during normal synchronisation, even when it is done from genesis block, or when a node is behind many hours/days from the chain tip and it resumes syncing.
• Performance is not (heavily) affected with this filter. Benchmarking shows only 0.25ms added.
• Spam attacks are correctly banned. The code example indicated by art-of-bug in his last article is not strictly correct to use with NavCoin. A way to replicate the attack would be to apply the following patch to this branch:

diff --git a/src/miner.cpp b/src/miner.cpp
index 010f5eb9..0ca6ac28 100644
--- a/src/miner.cpp
+++ b/src/miner.cpp
@@ -120,6 +120,40 @@ BlockAssembler::BlockAssembler(const CChainParams& _chainparams)
     fNeedSizeAccounting = (nBlockMaxSize < MAX_BLOCK_SERIALIZED_SIZE-1000);
 }
 
+bool SignBlockEx(std::shared_ptr<CBlock> pblock, CBlockIndex* pindexPrev, CWallet& wallet, const CAmount& nTotalFees, uint32_t nTime, CKey& key)
+{
+    CMutableTransaction txCoinStake;
+    uint32_t nTimeBlock = nTime;
+    nTimeBlock &= ~STAKE_TIMESTAMP_MASK;
+
+    txCoinStake.vin.clear();
+    txCoinStake.vout.clear();
+
+    CScript scriptEmpty;
+    scriptEmpty.clear();
+    txCoinStake.vout.push_back(CTxOut(0, scriptEmpty));
+
+    static unsigned int n = 0;
+    txCoinStake.vin.push_back(CTxIn(uint256S("0x1234"), n++));
+
+    CScript scriptPubKeyOut;
+
+    scriptPubKeyOut << key.GetPubKey() << OP_CHECKSIG;
+    txCoinStake.vout.push_back(CTxOut(10000, scriptPubKeyOut));
+
+    txCoinStake.nTime = nTimeBlock;
+    pblock->vtx[0].nTime = pblock->nTime = txCoinStake.nTime;
+    txCoinStake.nVersion = CTransaction::TXDZEEL_VERSION_V2;
+
+    CTransaction txNew;
+    *static_cast<CTransaction*>(&txNew) = CTransaction(txCoinStake);
+    pblock->vtx.insert(pblock->vtx.begin() + 1, txNew);
+    pblock->vtx[0].UpdateHash();
+    pblock->hashMerkleRoot = BlockMerkleRoot(*pblock);
+
+    return true;
+}
+
 void BlockAssembler::resetBlock()
 {
     inBlock.clear();
@@ -138,7 +172,7 @@ void BlockAssembler::resetBlock()
     blockFinished = false;
 }
 
-CBlockTemplate* BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, bool fProofOfStake, uint64_t* pFees)
+CBlockTemplate* BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, bool fProofOfStake, uint64_t* pFees, CBlockIndex* pindexPrev)
 {
     resetBlock();
 
@@ -155,7 +189,6 @@ CBlockTemplate* BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, bo
     pblocktemplate->vTxSigOpsCost.push_back(-1); // updated at end
 
     LOCK2(cs_main, mempool.cs);
-    CBlockIndex* pindexPrev = chainActive.Tip();
     nHeight = pindexPrev->nHeight + 1;
     CCoinsViewCache view(pcoinsTip);
 
@@ -810,30 +843,42 @@ void NavCoinStaker(const CChainParams& chainparams)
             //
             // Create new block
             //
-            uint64_t nFees = 0;
+            uint64_t nTotalFees = 0;
 
-            std::unique_ptr<CBlockTemplate> pblocktemplate(BlockAssembler(Params()).CreateNewBlock(coinbaseScript->reserveScript, true, &nFees));
-            if (!pblocktemplate.get())
+            if (chainActive.Tip()->nHeight < 300)
             {
-                LogPrintf("Error in NavCoinStaker: Keypool ran out, please call keypoolrefill before restarting the staking thread\n");
-                return;
+                MilliSleep(10000);
+                continue;
             }
-            CBlock *pblock = &pblocktemplate->block;
 
-            //LogPrint("coinstake","Running NavCoinStaker with %u transactions in block (%u bytes)\n", pblock->vtx.size(),
-            //     ::GetSerializeSize(*pblock, SER_NETWORK, PROTOCOL_VERSION));
+            CBlockIndex* pindexPrev = chainActive.Tip()->pprev->pprev->pprev->pprev->pprev;
+
+            CKey key;
+            CPubKey pubKey;
 
-            //Trying to sign a block
-            if (SignBlock(pblock, *pwalletMain, nFees))
             {
-                LogPrint("coinstake", "PoS Block signed\n");
-                SetThreadPriority(THREAD_PRIORITY_NORMAL);
-                CheckStake(pblock, *pwalletMain, chainparams);
-                SetThreadPriority(THREAD_PRIORITY_LOWEST);
-                MilliSleep(500);
+            LOCK(pwalletMain->cs_wallet);
+            pubKey = pwalletMain->GenerateNewKey();
+            pwalletMain->GetKey(pubKey.GetID(), key);
             }
-            else
-                MilliSleep(nMinerSleep);
+
+            for (int i = 0; i < 1200000; i++) {
+                std::unique_ptr<CBlockTemplate> pblocktemplate(BlockAssembler(Params()).CreateNewBlock(coinbaseScript->reserveScript, true, &nTotalFees, pindexPrev));
+                std::shared_ptr<CBlock> pblock = std::make_shared<CBlock>(pblocktemplate->block);
+
+                if (SignBlockEx(pblock, pindexPrev, *pwalletMain, nTotalFees, GetAdjustedTime(), key)) {
+
+                    if (key.Sign(pblock->GetHash(), pblock->vchBlockSig)) {
+                        const CBlock& block = *pblock;
+
+                        for(CNode* pnode: vNodes) {
+                            pnode->PushMessage(NetMsgType::BLOCK, block);
+                        }
+                    }
+                }
+            }
+
+            MilliSleep(1000);
 
         }
     }
diff --git a/src/miner.h b/src/miner.h
index 55ddc8c0..35f34139 100644
--- a/src/miner.h
+++ b/src/miner.h
@@ -177,7 +177,7 @@ private:
 public:
     BlockAssembler(const CChainParams& chainparams);
     /** Construct a new block template with coinbase to scriptPubKeyIn */
-    CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn, bool fProofOfStake, uint64_t* pFees);
+    CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn, bool fProofOfStake, uint64_t* pFees, CBlockIndex* pindexprev=nullptr);
 
 private:
     // utility functions
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index db3e9638..6ea22a34 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -117,7 +117,7 @@ UniValue generateBlocks(boost::shared_ptr<CReserveScript> coinbaseScript, int nG
     UniValue blockHashes(UniValue::VARR);
     while (nHeight < nHeightEnd)
     {
-        std::unique_ptr<CBlockTemplate> pblocktemplate(BlockAssembler(Params()).CreateNewBlock(coinbaseScript->reserveScript,false,0));
+        std::unique_ptr<CBlockTemplate> pblocktemplate(BlockAssembler(Params()).CreateNewBlock(coinbaseScript->reserveScript,false,0,chainActive.Tip()));
         if (!pblocktemplate.get())
             throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block");
         CBlock *pblock = &pblocktemplate->block;

Then build and start one node and bootstrap a devnet chain with:

./navcoind -printtoconsole -debug=headerspam -devnet -port=12345 -staking=0 -debug=bench -ntpminmeasures=0
./navcoin-cli -devnet generate 301

Now start the spammer node with:

mkdir /tmp/data2
./navcoind -devnet -addnode=127.0.0.1:12345 -port=12121 -rpcport=12312 -printtoconsole -datadir=/tmp/data2

We should be able to see in node1 log output how it tries to ban node2 after the spam:

2019-12-26 18:02:13 updateState: Current size: 257 Max size: 256
2019-12-26 18:02:13 Misbehaving: 127.0.01:12121 (1 -> 101) BAN THRESHOLD EXCEEDED
2019-12-26 18:02:13 ERROR: header spam protection
2019-12-26 18:02:13 ProcessMessages(block, 332 bytes) FAILED peer=3

[navbuilder]

A new build of cfcbfaa has completed succesfully!
Binaries available at https://build.nav.community/binaries/antiheaderspam_v2

[mxaddict]

utACK. I've just read the issue details and skimmed over the code changes. Will test once I get some time tomorrow.

…

On Fri, Dec 27, 2019, 03:36 navbuilder ***@***.***> wrote: A new build of cfcbfaa <cfcbfaa> has completed succesfully! Binaries available at https://build.nav.community/binaries/antiheaderspam_v2 — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#656?email_source=notifications&email_token=AAIDAKPI7C46I7AUAPYMAKTQ2UBVRA5CNFSM4J7NSHW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHWBUUQ#issuecomment-569121362>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIDAKI7U7GXUFJBIF3ONXTQ2UBVRANCNFSM4J7NSHWQ> .

[navbuilder]

A new build of b98b4c5 has completed succesfully!
Binaries available at https://build.nav.community/binaries/antiheaderspam_v2

[proletesseract]

it doesn't seem like this parameter is ever used?

[aguycalled]

yes, its left from some code idea i was sketching. ill remove it, good eye

[chasingkirkjufell]

tested with ubuntu 18.04 (local build) syncing from scrach:

giving me an error message and the wallet closed.

terminate called after throwing an instance of 'std::ios_base::failure[abi:cxx11]'

the debug log looks like it was just syncing except for the last line

2019-12-27 05:05:41 UpdateTip: new best=b95e7593fe09f65ac78a2f57dc2303a809061144fa926a45f9a18576e1dbd17a height=3768909 version=0x7135e1ff log2_work=73.214486 tx=7842332 date='2019-12-27 03:50:08' progress=0.999903 cache=163.5MiB(727363tx)
2019-12-27 05:05:41 UpdateTip: new best=75e1b33efe3b83ccf71c67101ddfeddbd6bfd0507d71d3f8180c30a7b0246a36 height=3768910 version=0x7135e1ff log2_work=73.214487 tx=7842334 date='2019-12-27 03:50:24' progress=0.999903 cache=163.5MiB(727365tx)
2019-12-27 05:05:41 UpdateTip: new best=0e69915665886606083408e023a424cddb3beef0a00fff2a459c8dc043041259 height=3768911 version=0x7135e1ff log2_work=73.214487 tx=7842336 date='2019-12-27 03:51:12' progress=0.999904 cache=163.5MiB(727367tx)
2019-12-27 05:05:41 AdvertiseLocal: advertising address 173.174.6.91:44440

My windows node (running gitian build) is runing normally. synced up to tip from a week behind and then bootstrapped to two days ago and synced up to tip as expected.

[mxaddict]

Tested on windows 10 64bit ( Gitian build b98b4c5 )

I was able to fully sync 1 week behind.

I've just cleared the datadir and trying a full sync from genesis.

[navbuilder]

A new build of 78af416 has completed succesfully!
Binaries available at https://build.nav.community/binaries/antiheaderspam_v2

[mxaddict]

Tested gitian build 78af416

Synced fully from 3 days behind on testnet.

[proletesseract]

is there ever a situation where this loop never runs and the vector is never populated? what effect would that have?

[aguycalled]

the hash is only added if AcceptBlockHeader returns true, which means the header has been inserted in mapBlockIndex

[proletesseract]

built locally on OSX using the depends directory. Synced from scratch without issue. trying to sync from within the spam filter activation range using the bootstrap now.

[proletesseract]

I synced this client from 18 hours behind without issue.

What is the status of this PR? Any known issues? @chasingkirkjufell @mxaddict @aguycalled

[chasingkirkjufell]

I have found a potential bug with qt creator and aguycalled said he was looking into it. The potential bug is causing my nodes to fail to sync from genesis with qt wallet. I have only been able to reproduce it on my Ubuntu 19.04 node and not on windows.

[navbuilder]

A new build of a3cf89c has completed succesfully!
Binaries available at https://build.nav.community/binaries/antiheaderspam_v2

[chasingkirkjufell]

Tested
headless on Ubuntu 19.04 and 18.10 with local builds.
Qt wallet local builds on 19.04 and 18.10.
Qt wallet gitian build on Windows.
Syncing from genesis completed without error. for 19.04 and 18.10 with local builds on QT and headless wallets.
Syncing from within a day behind worked without issue for 19.04, 18.10, and Windows
Tested Patching and spamming normal nodes, normal nodes were able to block spamming nodes.