Dirty read in "FastRawTransactionManager", variable should be defined volatile

[Robinron]

When troubleshooting an error where the transaction queue went up to 14 in stead of sending the transactions to pending and then mining them, my collegue noted this error that might be affecting the retrieval of nonce. See this part where Brian Goetz (java - chief architect) writes:

@ThreadSafe
public class CheesyCounter {
    // Employs the cheap read-write lock trick
    // All mutative operations MUST be done with the 'this' lock held
    @GuardedBy("this") private volatile int value;

    public int getValue() { return value; }

    public synchronized int increment() {
        return value++;
    }
}

ref: https://www.ibm.com/developerworks/java/library/j-jtp06197/index.html

So shouldnt the nonce be declared as volatile?

[nader-skatt]

One should consider to replace "synchronized" with

• either non-blocking alogrithms (e.g. AtomicReference)
• or ReentrantReadWriteLock (https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html)

Not only it will make the code faster, but makes it more readable.

[nader-skatt]

BTW, the code with potential bug is her:

https://github.com/web3j/web3j/blob/master/src/main/java/org/web3j/tx/FastRawTransactionManager.java#L11

As stated by Robinron, the instance variable should be made volatile. Otherwise, the getCurrentNonce() may read a dirty value.

[conor10]

I've changed to volatile. I'm going to release 2.2.3 shortly.

Would any of you be interested in submitting a PR using non-blocking logic? I've got a number of other enhancements I'm working on for the 3.0 release.

[nader-skatt]

I will send you a code. What is the deadline for submitting the code?

[conor10]

I'm going to get a minor release out today, but I'm keen to get web3j 3.0 out soon.

On 25 July 2017 at 11:19, nader-skatt notifications@github.com wrote:

I will send you a code. What is the deadline for submitting the code?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/web3j/web3j/issues/141#issuecomment-317694837, or mute the thread https://github.com/notifications/unsubscribe-auth/ACDHqnaRjECwQQ5AO5mOvum8CqAJCM9Nks5sRcEwgaJpZM4OhIV_ .

-- Web: http://conorsvensson.com Phone: +44 7497 376 365 Skype: conor_svensson

[mudlee]

@conor10 is that in the new release? If so, the release notes is wrong: https://github.com/web3j/web3j/releases/tag/v2.3.0 The last item is "Dirty read in "FastRawTransactionManager", variable should be defined volatile (#128)", but it points to a unrelated PR I guess.

[nader-skatt]

Here is a quick fix. Unfortunately, I cannot test the code as I do not have experience with Ethereum. I hope it helps :)

package org.web3j.tx;

import java.io.IOException;
import java.math.BigInteger;
import java.util.concurrent.atomic.AtomicReference;

import org.web3j.crypto.Credentials;
import org.web3j.protocol.Web3j;

/*
 * One should consider to make this class <code>final</code> such that the thread-safety is not damaged by a subclass.
 */
public final class NonblockingFastRawTransactionManager extends RawTransactionManager {

    private final AtomicReference<BigInteger> nonceHolder = new AtomicReference<>(BigInteger.valueOf(-1));

    public NonblockingFastRawTransactionManager(Web3j web3j, Credentials credentials, byte chainId) {
        super(web3j, credentials, chainId);
    }

    public NonblockingFastRawTransactionManager(Web3j web3j, Credentials credentials) {
        super(web3j, credentials);
    }

    @Override
    BigInteger getNonce() throws IOException {
        /*
         * According to JavaDoc, <code>BigInteger</code> is immutable:
         * "Immutable arbitrary-precision integers."
         * 
         * @see
         * https://docs.oracle.com/javase/7/docs/api/java/math/BigInteger.html
         * 
         * The algorithm is copied from this page:
         * https://stackoverflow.com/questions/14006495/possible-to-safely-
         * increment-biginteger-in-a-thread-safe-way-perhaps-with-atomi
         * 
         * See the code of incrementAndGet() in <code>AtomicLong</code>:
         * @see http://www.docjar.com/html/api/java/util/concurrent/atomic/AtomicLong.java.html 
         * 
         */

        for (;;) {
            final BigInteger currentNonce = nonceHolder.get();

            final BigInteger nextNonce;
            if (currentNonce.signum() == -1) {

                /*
                 * Are we sure that super.getNonce() returns the correct value
                 * in a thread-safe manner?
                 */
                nextNonce = super.getNonce();
            } else {
                nextNonce = currentNonce.add(BigInteger.ONE);
            }

            /*
             * Compete with other threads to set the value to
             * <code>nextNonce</code> if the current value is
             * <code>currentNonce</code>. If not successful, try it in the next
             * iteration of the loop.
             */
            if (nonceHolder.compareAndSet(currentNonce, nextNonce)) {
                return nextNonce;
            }
        }

    }

    public BigInteger getCurrentNonce() {
        return nonceHolder.get();
    }

    public void resetNonce() throws IOException {
        /*
         * Are we sure that super.getNonce() returns the correct value in a
         * thread-safe manner?
         */
        nonceHolder.set(super.getNonce());
    }

    public void setNonce(BigInteger value) {
        /*
         * This brutally sets a value. Perhaps, there should be some kind of
         * compareAndSet or accumulateAndSet?
         */
        nonceHolder.set(value);
    }
}
`

[nader-skatt]

How can I make a pull request to release 3.0?

[conor10]

@nader-skatt a regular pull request is fine.

[conor10]

I've been exploring this further - this is what a non-blocking implementation could look like.

The call to getCurrentNonce goes over a socket, so is much slower than the normal code paths of execution. Hence it's sensible to include synchronisation here.

The fundamental issue with these higher throughput approaches is that you need to ensure that all requests are delivered to the Ethereum node in sequence, which when we're calculating the nonces before sending the transaction always runs the risk of messages being received out of sequence.

Unfortunately due to the stateless nature of JSON-RPC this will remain a bottleneck. So I'm not certain if the below would be of use to the users of the library.

public final class NonblockingFastRawTransactionManager extends RawTransactionManager {

    private static final BigInteger INIT_VALUE = BigInteger.valueOf(-1);

    private final AtomicReference<BigInteger> nonceHolder =
            new AtomicReference<>(INIT_VALUE);

    public NonblockingFastRawTransactionManager(Web3j web3j, Credentials credentials, byte chainId) {
        super(web3j, credentials, chainId);
    }

    public NonblockingFastRawTransactionManager(Web3j web3j, Credentials credentials) {
        super(web3j, credentials);
    }

    @Override
    BigInteger getNonce() throws IOException {

        for (;;) {
            final BigInteger currentNonce = nonceHolder.get();

            final BigInteger nextNonce;
            if (currentNonce.signum() == -1) {

                synchronized (this) {
                    if (currentNonce.signum() == -1) {
                        nextNonce = super.getNonce();
                    } else {
                        continue;
                    }
                }
            } else {
                nextNonce = currentNonce.add(BigInteger.ONE);
            }

            if (nonceHolder.compareAndSet(currentNonce, nextNonce)) {
                return nextNonce;
            }
        }

    }

    public BigInteger getCurrentNonce() {
        return nonceHolder.get();
    }

    public void resetNonce() throws IOException {
        setNonce(INIT_VALUE);
    }

    public void setNonce(BigInteger value) {
        for (;;) {
            if (nonceHolder.compareAndSet(value, value)) {
                break;
            }
        }
    }
}