XRPLF / rippled

Decentralized cryptocurrency blockchain daemon implementing the XRP Ledger protocol in C++
https://xrpl.org
ISC License
4.52k stars 1.46k forks source link

[BUG] OrderBook update logic typo #4790

Open sublimator opened 1 year ago

sublimator commented 1 year ago
void
OrderBookDB::setup(std::shared_ptr<ReadView const> const& ledger)
{
    if (!app_.config().standalone() && app_.getOPs().isNeedNetworkLedger())
    {
        JLOG(j_.warn()) << "Eliding full order book update: no ledger";
        return;
    }

    auto seq = seq_.load();

    if (seq != 0)
    {
        if ((seq > ledger->seq()) && ((ledger->seq() - seq) < 25600))
            return;

        if ((ledger->seq() <= seq) && ((seq - ledger->seq()) < 16))
            return;
    }

This part seems a bit confusing to me:

        if ((seq > ledger->seq()) && ((ledger->seq() - seq) < 25600))

If bigger > smaller && (smaller - bigger) < 25600

The old logic:

        auto seq = ledger->info().seq;

        // Do a full update every 256 ledgers
        if (mSeq != 0)
        {
            if (seq == mSeq)
                return;
            if ((seq > mSeq) && ((seq - mSeq) < 256))
                return;
            if ((seq < mSeq) && ((mSeq - seq) < 16))
                return;
        }
sublimator commented 1 year ago

Maybe it was supposed to be:

if ((ledger->seq() > seq) && ((ledger->seq() - seq) < 25600))

I see before the variable seq was used for the passed in ledger' sequence, but now seq is the last sequence (which before was mSeq) so maybe there was some copy/paste issues?

seelabs commented 9 months ago

Thanks @sublimator. After a quick look this does look like a bug to me. Let me take a closer look now.

seelabs commented 9 months ago

@sublimator There's a PR for this here: https://github.com/XRPLF/rippled/pull/4890