Fix to use vsize for mempool fee histogram (#225)

* base fix
* Refactored. Also fixed bug where vsize was 4x
* more refactoring
* Added some Txn ser/deser testing
* Improved the frequency of mempool fee histogram updates
  - We now update the mempool fee histogram every 10 seconds, instead of
    every 30
 - Whenever we get a new block, we update it immediately upon the first
    mempool synch after the block arrives

  This should improve the txn estimates in Electrum for BTC users.

* Dont print Storage::refreshMempoolHistogram elapsed time unless >= 10msec

Also do the printing with no locks held, and destruct the swapped-in
hist vec with no locks held.

* Tweak to mempool bench to populate vsize correctly
* Nit

Author: cculianu

src/Controller.cpp

@@ -257,15 +257,27 @@ void Controller::startup()
     {
         // set up periodic refresh of mempool fee histogram
         constexpr const char *feeHistogramTimer = "feeHistogramTimer";
-        constexpr int feeHistogramTimerInterval = 30 * 1000; // every 30 seconds
+        constexpr int feeHistogramTimerInterval = 10 * 1000; // every 10 seconds
         conns += connect(this, &Controller::upToDate, this, [this] {
             callOnTimerSoon(feeHistogramTimerInterval, feeHistogramTimer, [this]{
                 storage->refreshMempoolHistogram();
                 return true;
-            }, false, Qt::TimerType::VeryCoarseTimer);
+            }, false, Qt::TimerType::CoarseTimer);
+        });
+        conns += connect(this, &Controller::synchedMempool, this, [this]{
+            // If we just synched the mempool after a new block arrived, or for the first time after app start,
+            // refresh the fee histogram immediately.
+            if (needFeeHistogramUpdate) {
+                needFeeHistogramUpdate = false;
+                storage->refreshMempoolHistogram();
+                restartTimer(feeHistogramTimer);
+            }
         });
         // disable the timer if downloading blocks and restart it later when up-to-date
-        conns += connect(this, &Controller::synchronizing, this, [this]{ stopTimer(feeHistogramTimer); });
+        conns += connect(this, &Controller::synchronizing, this, [this]{
+            stopTimer(feeHistogramTimer);
+            needFeeHistogramUpdate = true; // indicate that the next time synchedMempool() is emitted, do a fee histogram refresh
+        });
     }
 
     {
@@ -1106,11 +1118,14 @@ void Controller::process(bool beSilentIfUpToDate)
         // ...
         if (bitcoindmgr->hasDSProofRPC())
             sm->state = State::SynchDSPs; // remote bitcoind has dsproof rpc, proceed to synch dsps
-        else
+        else {
+            emit synchedMempool();
             sm->state = State::End; // remote bitcoind lacks dsproof rpc, finish successfully
+        }
         AGAIN();
     } else if (sm->state == State::SynchDSPsFinished) {
         // ...
+        emit synchedMempool();
         sm->state = State::End;
         AGAIN();
     }

src/Controller.h

@@ -85,11 +85,11 @@ class Controller : public Mgr, public ThreadObjectMixin, public TimersByNameMixi
     }
 
 signals:
-    /// Emitted whenever bitcoind is detected to be up-to-date, and everything is synched up.
+    /// Emitted whenever bitcoind is detected to be up-to-date, and everything (except mempool) is synched up.
     /// note this is not emitted during regular polling, but only after `synchronizing` was emitted previously.
     void upToDate();
-    /// Emitted whenever we begin synching to bitcoind. After this completes successfully, upToDate will be emitted
-    /// exactly once.
+    /// Emitted whenever we begin dl'ing blocks from bitcoind. After this completes successfully, upToDate will be
+    /// emitted exactly once.
     /// This signal may be emitted multiple times if there were errors and we are periodically retrying.
     void synchronizing();
     /// Emitted whenever we failed to synchronize to bitcoind.
@@ -117,6 +117,10 @@ class Controller : public Mgr, public ThreadObjectMixin, public TimersByNameMixi
     /// Emitted whenever we begin downloading blocks, and whenever we end the last DownloadBlocksTask (for whatever reason)
     void downloadingBlocks(bool b);
 
+    /// Emitted after a successful mempool synch. This is emitted more often than upToDate() (which is only emitted
+    /// when new blocks arrive); this is emitted every time we successfully poll the mempool for new txns.
+    void synchedMempool();
+
 protected:
     Stats stats() const override; // from StatsMixin
     Stats debug(const StatsParams &) const override; // from StatsMixin
@@ -218,6 +222,9 @@ protected slots:
     /// for that block, until a new block arrives, then is cleared again.
     std::unordered_set<TxHash, HashHasher> mempoolIgnoreTxns;
 
+    /// Used to update the mempool fee histogram early right after the synchedMempool() signal is emitted
+    bool needFeeHistogramUpdate = true;
+
 private slots:
     /// Stops the zmqHashBlockNotifier; called if we received an empty hashblock endpoint address from BitcoinDMgr or
     /// when all connections to bitcoind are lost

src/Controller_SynchMempoolTask.cpp

@@ -477,9 +477,13 @@ void SynchMempoolTask::doDLNextTx()
                 return;
             }
 
-            // save size now -- this is needed later to calculate fees and for everything else.
-            // note: for btc core with segwit this size is not the same "virtual" size as what bitcoind would report
+            // Save size now -- this is needed later to calculate fees and for everything else.
             tx->sizeBytes = unsigned(expectedLen);
+            if (isSegWit) {
+                tx->vsizeBytes = ctx.GetVirtualSize(tx->sizeBytes);
+            } else {
+               tx->vsizeBytes = tx->sizeBytes;
+            }
 
             if (TRACE)
                 Debug() << "got reply for tx: " << hashHex << " " << txdata.length() << " bytes";

src/Mempool.cpp

@@ -46,8 +46,8 @@ auto Mempool::calcCompactFeeHistogram(unsigned binSizeBytes) const -> FeeHistogr
     for (const auto & [txid, tx] : txs) {
         if (tx->fee < bitcoin::Amount::zero()) continue; // skip negative fees (coinbase txn, etc)
         const auto feeRate = unsigned(tx->fee / bitcoin::Amount::satoshi()) // sats
-                             /  std::max(tx->sizeBytes, 1u); // per byte
-        histogram[feeRate] += tx->sizeBytes; // accumulate size by feeRate
+                             / std::max(tx->vsizeBytes, 1u); // per vbyte
+        histogram[feeRate] += tx->vsizeBytes; // accumulate size by feeRate
     }
 
     // Now, compact the bins
@@ -881,7 +881,7 @@ namespace {
     std::pair<MPData, bool> loadMempoolDat(const QString &fname) {
         // Below code taken from BCHN validation.cpp, but adopted to also support segwit
         FILE *pfile = std::fopen(fname.toUtf8().constData(), "rb");
-        bitcoin::CAutoFile file(pfile, bitcoin::SER_DISK, bitcoin::PROTOCOL_VERSION | bitcoin::SERIALIZE_TRANSACTION_USE_WITNESS);
+        bitcoin::CAutoFile file(pfile, bitcoin::SER_DISK, bitcoin::PROTOCOL_VERSION | bitcoin::SERIALIZE_TRANSACTION_USE_WITNESS | bitcoin::SERIALIZE_TRANSACTION_USE_CASHTOKENS);
         if (file.IsNull())
             throw Exception(QString("Failed to open mempool file '%1'").arg(fname));
 
@@ -912,7 +912,8 @@ namespace {
 
                 auto rtx = std::make_shared<Mempool::Tx>();
                 rtx->hash = BTC::Hash2ByteArrayRev(ctx->GetHashRef());
-                dataTotal += rtx->sizeBytes = ctx->GetTotalSize(true);
+                dataTotal += rtx->sizeBytes = ctx->GetTotalSize(isSegWit, false);
+                rtx->vsizeBytes = isSegWit ? ctx->GetVirtualSize(rtx->sizeBytes) : rtx->sizeBytes;
                 ret.emplace(std::piecewise_construct,
                             std::forward_as_tuple(rtx->hash),
                             std::forward_as_tuple(std::move(rtx), std::move(ctx)));

src/Mempool.h

@@ -48,7 +48,8 @@ struct Mempool
         TxHash hash; ///< in reverse bitcoind order (ready for hex encode), fixed value.
 
         bitcoin::Amount fee{bitcoin::Amount::zero()}; ///< we calculate this fee ourselves since in the past I noticed we get a funny value sometimes that's off by 1 or 2 sats --  which I suspect is due limitations of doubles, perhaps?
-        unsigned sizeBytes = 0;
+        unsigned sizeBytes = 0; // the actual serialized size
+        unsigned vsizeBytes = 0; // for segwit coins, the virtual size, for non-segwit, identical to sizeBytes
         bool hasUnconfirmedParentTx = false; ///< If true, this tx depends on another tx in the mempool. This is not always fixed (confirmedInBlock may change this)
 
         /// These are all the txos in this tx. Once set-up, this doesn't change (unlike IOInfo.utxo).

src/Mixins.cpp

@@ -105,7 +105,7 @@ void TimersByNameMixin::callOnTimerSoon(int ms, const QString &name, const std::
 {
     if (auto it = _timerMap.find(name); it != _timerMap.end()) {
         if (force) {
-            it->get()->stop(); // immediately stop timer
+            it.value()->stop(); // immediately stop timer
             it = _timerMap.erase(it); // shared_ptr refs will go away immediately, which ends up calling deleteLater on timer
             callOnTimerSoon(ms, name, func, false, ttype); // immediately call self recursively once to re-enqueue timer
         }
@@ -164,6 +164,15 @@ QVariantMap TimersByNameMixin::activeTimerMapForStats() const
     return ret;
 }
 
+bool TimersByNameMixin::restartTimer(const QString &name)
+{
+    if (auto it = _timerMap.find(name); it != _timerMap.end()) {
+        it.value()->start(); // restart from "now"
+        return true;
+    }
+    return false;
+}
+
 /// --- StatsMixin
 StatsMixin::~StatsMixin() {}
 

src/Mixins.h

@@ -172,6 +172,10 @@ class TimersByNameMixin : public virtual QObjectMixin
     /// Stops all extant timers immediately.  Note that this implicitly uses deleteLater to delete the timer objects.
     /// Returns the number of timers that were stopped.
     int stopAllTimers();
+
+    /// Stops the named timer, and immediately restarts it with the same interval and oneshot status, effectively
+    /// re-offsetting the next time the timer will run as offset from *now*.
+    bool restartTimer(const QString &name);
 };
 
 

src/Servers.cpp

@@ -2707,14 +2707,21 @@ void AdminServer::rpc_getinfo(Client *c, const RPC::BatchId batchId, const RPC::
         auto [mempool, lock] = storage->mempool();
         mp["txs"] = qulonglong(mempool.txs.size());
         mp["addresses"] = qulonglong(mempool.hashXTxs.size());
-        std::size_t sizeTotal = 0;
+        std::size_t sizeTotal = 0, vsizeTotal = 0;
         std::int64_t feeTotal = 0;
-        std::for_each(mempool.txs.begin(), mempool.txs.end(), [&sizeTotal, &feeTotal](const auto &pair){
-            sizeTotal += pair.second->sizeBytes;
-            feeTotal += pair.second->fee / bitcoin::Amount::satoshi();
+        std::for_each(mempool.txs.begin(), mempool.txs.end(), [&sizeTotal, &vsizeTotal, &feeTotal](const auto &pair){
+            const auto & [_, tx] = pair;
+            sizeTotal += tx->sizeBytes;
+            vsizeTotal += tx->vsizeBytes;
+            feeTotal += tx->fee / bitcoin::Amount::satoshi();
         });
         mp["size_bytes"] = qulonglong(sizeTotal);
         mp["avg_fee_sats_B"] = sizeTotal ? long(std::round(double(feeTotal) / double(sizeTotal) * 100.0)) / 100.0 : 0.0;
+        if (sizeTotal != vsizeTotal) {
+            // add these elements to the info map if BTC, LTC, etc
+            mp["size_vbytes"] = qulonglong(vsizeTotal);
+            mp["avg_fee_sats_vB"] = vsizeTotal ? long(std::round(double(feeTotal) / double(vsizeTotal) * 100.0)) / 100.0 : 0.0;
+        }
         res["mempool"] = mp;
     }
     { // utxoset

src/Storage.cpp

@@ -3991,16 +3991,19 @@ auto Storage::mutableMempool() -> std::pair<Mempool &, ExclusiveLockGuard>
 
 void Storage::refreshMempoolHistogram()
 {
+    Tic t0;
     Mempool::FeeHistogramVec hist;
+    // shared lock
     {
-        // shared lock
         auto [mempool, lock] = this->mempool();
-        auto histTmp = mempool.calcCompactFeeHistogram();
-        hist.swap(histTmp);
+        hist = mempool.calcCompactFeeHistogram();
     }
     // lock exclusively to do the final swap
-    ExclusiveLockGuard g(p->mempoolLock);
-    p->mempoolFeeHistogram.swap(hist);
+    {
+        ExclusiveLockGuard g(p->mempoolLock);
+        p->mempoolFeeHistogram.swap(hist);
+    }
+    if (t0.msec() >= 10) DebugM("Storage::refreshMempoolHistogram took ", t0.msecStr(), " msec");
 }
 
 auto Storage::mempoolHistogram() const -> Mempool::FeeHistogramVec

src/bitcoin/interpreter.cpp

@@ -1504,8 +1504,7 @@ uint256 GetOutputsHash(const CTransaction &txTo) {
 
 } // namespace
 
-PrecomputedTransactionData::PrecomputedTransactionData(
-    const CTransaction &txTo) {
+PrecomputedTransactionData::PrecomputedTransactionData(const CTransaction &txTo) {
     hashPrevouts = GetPrevoutHash(txTo);
     hashSequence = GetSequenceHash(txTo);
     hashOutputs = GetOutputsHash(txTo);