0.97-RC1: two zc-parser crashes on obfuscated block files

[tomrobbo]

Hit these two running DB manually on RC1 from source on aarch64 against an obfuscated Core 31 datadir - suggested fixes below, both tested.

1. getRawBlockData truncates xor key to 1 byte

BlockchainData.cpp, xor branch:

for (auto& chunk : xoredData)   // uint8_t&
   chunk ^= xorkey;             // ^= (uint8_t)xorkey  -> key[0] only

Garbage for any non-uniform key -> deserialize throws -> failed to grab tx.
Scan path (FileCopy::xorMe) is correct, so only on-demand reads (getTx, zc
parse) break. Suggested fix:

-      for (auto& chunk : xoredData)
-         chunk ^= xorkey;
+      auto data64 = reinterpret_cast<uint64_t*>(xoredData.data());
+      for (size_t i = 0; i < xoredData.size() / 8; i++)
+         data64[i] ^= xorkey;

2. zc parser aborts on unresolved parent

In preprocessTx, getTxOutCopy throws range_error ("index out of bound:
1879 out of 3"), no barrier up to the thread loop -> terminate. range_error is
caught elsewhere in the module but not here. Suggested fix:

-      auto parentTx = bd->getTx(opRef.getDbKey());
-      auto txOut = parentTx.getTxOutCopy(opRef.getIndex());
-      txIn.scrAddr = txOut.getScrAddress();
-      txIn.value = txOut.getAmount();
+      try {
+         auto parentTx = bd->getTx(opRef.getDbKey());
+         auto txOut = parentTx.getTxOutCopy(opRef.getIndex());
+         txIn.scrAddr = txOut.getScrAddress();
+         txIn.value = txOut.getAmount();
+      } catch (const std::exception& e) {
+         LOGWARN << "preprocessTx: skipping input " << iin << ": " << e.what();
+         continue;
+      }

Both verified on aarch64: 1 by moving past where it crashed originally, 2 by fault injection.

[goatpig]

Issue 1:
Dealt with in bb32464

Issue 2:
This is too broad, you need to narrow the bug conditions.

First please provide a file:line when reporting, to avoid ambiguity. Second, describe the type of zeroconf this is happening for: were you sending coins? Expecting coins? Monitoring some 3rd party address? None of the above? This category of issue needs to be reproduced in a test first, before it can be addressed.

In most cases, throws are left unhandled in the ZC parser on purpose. A missing parent can be a critical condition: it is likely worse to ignore the issue than to let the DB run. To silently suppress a missed parent is to tell to the user that the system is sane while the effect of a potentially critical ZC is not being represented.

[tomrobbo]

Issue 1: great

Issue 2: Closest to none of the above - a wallet's loaded but it has no activity, so the tx wasn't related to it. I wasn't sending or expecting anything.

file:line - is Tx::getTxOutCopy at TxClasses.cpp:772, called from preprocessTx at Utils.cpp:243.

Message was index out of bound: 1879 out of 3 (output 1879 on a 2-output parent). Fair enough on leaving it unhandled, that makes sense. If I can catch it again I'll let you know.

[goatpig]

I will review the code with that condition in mind, maybe something will come out of it.

[goatpig]

I think the second case is what happens when the code gets past issue 1. A mangled tx (due to bad blockxor demasking) will have bogus varint counts. It shouldn't happen anymore with proper demasking. Please let me know if you're still experiencing this crash, otherwise close the issue.

[tomrobbo]

I think the second case is what happens when the code gets past issue 1. A mangled tx (due to bad blockxor demasking) will have bogus varint counts. It shouldn't happen anymore with proper demasking. Please let me know if you're still experiencing this crash, otherwise close the issue.

That makes sense - thanks for looking into it

[tomrobbo]

Reopening - still hitting case 2 on rc2_fixes (667c1b6). Low priority given your DB_FULL note on forum (I misunderstood I thought I was supposed to be using FULL).

It aborted (DB_FULL, --public, on c39219b at the time, ~50 min after startup):

terminate called after throwing an instance of 'std::range_error'
  what():  index out of bound: 8 out of 2

To see what it was resolving I put a catch + LOGERR at the getTxOutCopy call locally. ~20 hits in 48h of uptime. In every one, the outpoint hash and the parent it resolved to share their last 8 hex chars and nothing else:

preprocessTx: index out of bound: 11 out of 3 | outpoint 64b7b360f9526749e1707fe1557929a4c738c691a5e0b22cc7d428fe302d97a1:11 -> parent 6668e3266cd2026187d0fd2eee4d3e562a4b9c57db1a8d2c78c7c449302d97a1 (2 outputs)
preprocessTx: index out of bound: 2 out of 3  | outpoint 5a3d14f1d74f6342fca92e012ef6acb40c4a8735fb55d3b5927150ada570c52a:2  -> parent 4524d8c98a4b6f68f47313fabfb35da09706ec1e4fbb38b5876dac13a570c52a (2 outputs)
preprocessTx: index out of bound: 1 out of 2  | outpoint aa5567fabeca44359b8892c8406ce41f8c6adda7ff8fe0e9f16e1a482e3ed359:1  -> parent 5919ffef372bca2598e21bafee306c16e7bf9313659dd5e98e34fd352e3ed359 (1 outputs)

One more thing from the same log: 64b7b360...302d97a1 gave these errors from 13:58 to 14:47, then from 14:58 it shows up as a resolved TxHintCollision instead and the errors stop.

Will switch my day-to-day node to bare; keeping the full DB if you want anything re-tested. The --db-type help text still says DB_FULL is the default

[goatpig]

Ah, maybe I know what's going on. The ZC parser tries to resolve outpoint hashes from db first (which only carries mined hashes). If a ZC is spending from a ZC (i.e. an outpoint that isn't mined yet), and that outpoint hash collides with a mined hash, this out of order resolution will point to an invalid parent.

This is because the parser does not check if the hash of the parent matches, it trusts the db hit fully. If it was to check the pool of ZC hashes first, this may work a whole lot better. I will write a test to reproduce this case and implement a fix accordingly. Don't get rid of your DB_FULL instance just yet!

Regarding DB_FULL:

1. It eats too much RAM. This is in part due to LMDB which mmaps of all of its files, and also in part due to the threading model. It grabs as many threads at it needs, which eats a bunch of RAM and gets in the way of using a static scratchpad. I have to implement a static task scheduler with a fixed amount of threads. An added bonus will be --thread-count and --ram-usage having a predictable effect.
2. LMDB itself struggles starting millions of records per db. It slows down and the files grow too big. I have a hash table into binary tree code to replace it for these large records (~1.5B tx hashes in mainnet). It;s about 90% of the way, it should keep hints db and later address db (for supernode, ~5B addresses) down to more manageable sizes, besides being a few times faster than LMDB.

None of this is in scope for 0.97, this is more of a 0.97.1 target. For now I'm just hoping to deliver a stable DB_BARE (which purpose is to run on anything that can sync Core within a week or so).

[goatpig]

Latest commit in rc2_fixes (47ab5f5) should fix the issue.

[tomrobbo]

Latest commit in rc2_fixes (47ab5f5) should fix the issue.

Have got it running, will let you know