Proposed 0.70.2 hotfix #2231

ximinez · 2017-09-19T18:33:47Z

Recover old open ledger transactions to the queue:

Recover to the open ledger once, then to the queue.
If transaction fails to queue for any reason, drop it.
New result codes for transactions that can not queue.
Add minimum queue size
RIPD-1530
fix Recent fee rises and TxQ issues #2215

ximinez · 2017-09-19T19:32:40Z

Build and test problems. Closing to fix.

nbougalis · 2017-09-19T19:06:43Z

src/ripple/app/ledger/impl/OpenLedger.cpp

+        if (app.getHashRouter().shouldRecover(tx->getTransactionID()))
+            return ripple::apply(
+                app, view, *tx, flags, j);
+        else


Sorry to nitpick, but please use {} here. Yes, it's not strictly necessary, but the comment block following the else is large and the curlies will help make the code easier to read.

Fixed, and entirely sensible.

nbougalis · 2017-09-19T19:06:57Z

src/ripple/app/ledger/impl/OpenLedger.cpp

+    auto const result = [&]
+    {
+        if (app.getHashRouter().shouldRecover(tx->getTransactionID()))
+            return ripple::apply(


micro-nit: single line works fine here.

nbougalis · 2017-09-19T19:32:57Z

src/ripple/app/misc/HashRouter.h

+            @note The limit is signed while the counter is unsigned.
+                A negative limit will retry forever.
+        */
+        bool shouldRecover(std::int32_t limit)


If we don't care about negative limits (why should we?) then this can be neatly simplified as the following, if we increase limit by 1:

bool shouldRecover(std::int32_t limit) { return ++recoveries_ % limit != 0; }

This reads better, at least to me. And, as an extra benefit, it tracks exactly how many times we've tried to recover the item.

Also, I'd also make both limit and recoveries_ be unsigned int.

I wanted to leave the option available to basically remove the limitations by setting a negative limit.

I also wanted an option to have a limit of 0, which would basically prevent retries entirely.

If we don't want either of those, your optimization makes sense (and was my initial approach, too). (I did have another concern about integer overflow, but if a tx retries 2^32 times, that's a whole other issue.)

Per discussion, changed for the simplification

nbougalis · 2017-09-19T20:00:15Z

src/ripple/app/consensus/RCLConsensus.cpp

@@ -303,6 +308,7 @@ RCLConsensus::onClose(
    // Build SHAMap containing all transactions in our open ledger
    for (auto const& tx : initialLedger->txs)
    {
+      JLOG(j_.debug()) << "Adding open ledger TX " << tx.first->getTransactionID();


Micro-nit: indentation

nbougalis · 2017-09-19T20:11:11Z

src/ripple/app/misc/impl/TxQ.cpp

        canBeHeld = accountIter == byAccount_.end() ||
            replacementIter ||
                accountIter->second.getTxnCount() <
-                    setup_.maximumTxnPerAccount;
+                    setup_.maximumTxnPerAccount ||


This construct has now, officially, become impossible to read.

Is this better maybe?

/* Limit the number of transactions an individual account can queue. Mitigates the lost cost of relaying should an early one fail or get dropped. */ if (canBeHeld) { if (accountIter != byAccount_.end()) canBeHeld = false; if (!canBeHeld && replacementIter) canBeHeld = true; if (!canBeHeld && accountIter->second.getTxnCount() < setup_.maximumTxnPerAccount) canBeHeld = true; // Allow the transaction to get in front of the first queued // transaction. This allows recovery of open ledger transactions // and stuck transactions. if (!canBeHeld && tx.getSequence() < accountIter->second.transactions.begin()->first) canBeHeld = true; }

P.S.: I am convinced that the above logic is correct and equivalent to what's already there, but please double-check before using this.

I updated it differently, but I think you'll like it.

nbougalis

Please address the issue with bool shouldRecover(std::uint32_t limit)

nbougalis · 2017-09-20T03:51:24Z

src/ripple/app/consensus/RCLConsensus.cpp

@@ -163,6 +164,10 @@ RCLConsensus::relay(RCLCxTx const& tx)
        app_.overlay().foreach (send_always(
            std::make_shared<Message>(msg, protocol::mtTRANSACTION)));
    }
+    else
+    {
+        JLOG(j_.debug()) << "Not relaying disputedtx " << tx.id();


space between disputed and tx

nbougalis · 2017-09-20T04:17:20Z

src/ripple/app/ledger/impl/OpenLedger.cpp

+            Serializer s;
+
+            tx->add(s);
+            msg.set_rawtransaction(&s.getData().front(), s.getLength());


You can do:
msg.set_rawtransaction(s.data(), s.size());

nbougalis · 2017-09-20T04:36:46Z

src/ripple/app/misc/HashRouter.h

+        */
+        bool shouldRecover(std::uint32_t limit)
+        {
+            return ++recoveries_ % limit != 0;


So getDefaultRecoverLimit returns 1 and since x mod 1 is always 0 this will always return false. If we make getDefaultRecoverLimit return 2, then this function will oscillate between true and false. Not sure if that's what we want.

I know that I was the one that suggested this change, but I'm now thinking that this should be reduced to:

bool shouldRecover(std::uint32_t limit) { return recoveries_++ < limit; }

With limit == 1 this function will return to the sequence true, false, false, false, ...

HashRouter (Stopwatch& clock, std::chrono::seconds entryHoldTimeInSeconds, std::uint32_t recoverLimit) : suppressionMap_(clock) , holdTime_ (entryHoldTimeInSeconds) , recoverLimit_ (recoverLimit + 1u) { }

That does the right thing, works after rediscovery, and allows the API to specify "recover this many times before giving up."

nbougalis · 2017-09-20T04:39:03Z

src/ripple/app/misc/impl/TxQ.cpp

-                    setup_.maximumTxnPerAccount;
+
+        // Allow if the account is not in the queue at all
+        canBeHeld = accountIter == byAccount_.end();


Thanks! This is much more readable!

wilsonianb · 2017-09-19T22:50:41Z

src/ripple/app/misc/HashRouter.h

+        /** Determines if this item should be recovered from the open ledger.
+
+            Counts the number of times the item has been recovered.
+            If it hits the limit, reset the counter and return false.


~~reset the counter~~

wilsonianb · 2017-09-20T03:32:59Z

src/ripple/app/ledger/impl/OpenLedger.cpp

+                // already had a chance via disputes
+                hint->second = false;
+            else
+                shouldRecover.emplace_hint(hint, txID,


why emplace_hint if hint is end()? Is it because we know this is the highest tx added from current_ (even though there may be larger disputed tx ids in the map)?

Good catch. I mixed my metaphors and used the wrong function. Should be right now.

wilsonianb · 2017-09-21T18:30:24Z

src/ripple/app/misc/impl/TxQ.cpp

@@ -1454,6 +1482,7 @@ setup_TxQ(Config const& config)
    TxQ::Setup setup;
    auto const& section = config.section("transaction_queue");
    set(setup.ledgersInQueue, "ledgers_in_queue", section);
+    set(setup.queueSizeMin, "minimum_queue_size", section);


Should this be documented in rippled-example.cfg

Probably, yeah.

wilsonianb · 2017-09-21T18:31:52Z

src/ripple/protocol/impl/TER.cpp

+        { telCAN_NOT_QUEUE_BLOCKS,   { "telCAN_NOT_QUEUE_BLOCKS",  "Can not queue at this time: would block later queued transaction(s)." } },
+        { telCAN_NOT_QUEUE_BLOCKED,  { "telCAN_NOT_QUEUE_BLOCKED", "Can not queue at this time: blocking transaction in queue." } },
+        { telCAN_NOT_QUEUE_FEE,      { "telCAN_NOT_QUEUE_FEE",     "Can not queue at this time: fee insufficient to replace queued transaction." } },
+        { telCAN_NOT_QUEUE_FULL,     { "telCAN_NOT_QUEUE_FULL",    "Can not queue at this time: queue is full." } },


closing curly braces aren't aligned with the rest of the file

Doesn't really help readability for me, since the lines are already so insane long, but fixed.

* If the transaction can't be queued, recover to the open ledger once, and drop it on the next attempt. * New result codes for transactions that can not queue. * Add minimum queue size. * Remove the obsolete and incorrect SF_RETRY flag. * fix XRPLF#2215

ximinez force-pushed the relayopenledger.70 branch 2 times, most recently from b2c08e5 to 8c2fd48 Compare September 19, 2017 18:51

ximinez closed this Sep 19, 2017

nbougalis reviewed Sep 19, 2017

View reviewed changes

ximinez reopened this Sep 19, 2017

ximinez force-pushed the relayopenledger.70 branch 2 times, most recently from 1368040 to 86892cd Compare September 19, 2017 22:54

nbougalis suggested changes Sep 20, 2017

View reviewed changes

wilsonianb reviewed Sep 20, 2017

View reviewed changes

ximinez force-pushed the relayopenledger.70 branch from 78ceaec to 50ee48b Compare September 20, 2017 20:00

wilsonianb reviewed Sep 21, 2017

View reviewed changes

ximinez added 2 commits September 21, 2017 15:02

Set version to 0.70.2

cd2d52a

ximinez force-pushed the relayopenledger.70 branch from 58ca7b7 to cd2d52a Compare September 21, 2017 19:11

nbougalis merged commit cd2d52a into XRPLF:master Sep 21, 2017

ximinez deleted the relayopenledger.70 branch September 21, 2017 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposed 0.70.2 hotfix #2231

Proposed 0.70.2 hotfix #2231

ximinez commented Sep 19, 2017

ximinez commented Sep 19, 2017

nbougalis Sep 19, 2017

ximinez Sep 19, 2017

nbougalis Sep 19, 2017

ximinez Sep 19, 2017

nbougalis Sep 19, 2017

ximinez Sep 19, 2017

ximinez Sep 19, 2017

nbougalis Sep 19, 2017

ximinez Sep 19, 2017

nbougalis Sep 19, 2017

ximinez Sep 19, 2017

nbougalis left a comment

nbougalis Sep 20, 2017

ximinez Sep 20, 2017

nbougalis Sep 20, 2017

nbougalis Sep 20, 2017

ximinez Sep 20, 2017

nbougalis Sep 20, 2017

wilsonianb Sep 19, 2017

ximinez Sep 20, 2017

wilsonianb Sep 20, 2017

ximinez Sep 20, 2017

wilsonianb Sep 21, 2017

ximinez Sep 21, 2017

wilsonianb Sep 21, 2017

ximinez Sep 21, 2017

Proposed 0.70.2 hotfix #2231

Proposed 0.70.2 hotfix #2231

Conversation

ximinez commented Sep 19, 2017

ximinez commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment