Integrate support for 0-conf

[rafa-js]

Overview

As you know, Bitcoin Cash dropped the legacy RBF protocol in order to simplify 0-conf payments. Still not 100% secure since a rival could try different attacks that I personally consider very unlikely, but just my opinion (we can discuss about it if required).

The different Bitcoin Cash wallet, should consider this feature to provide the best user experience. In the future, we should stop using "confirmations" as a common word for payments. To go mainstream we need people and merchants to stop caring about confirmations, they just need to pay done. Does people care about the technical details when they pay using Visa? Not at all.

Since we have a technology that allow us to work this way, I think we should start taking the most from Bitcoin Cash. If we all are integrating 0-conf in our wallets, add this feature to the Bitcoinj-cash library is the way to go: each wallet don't need to reinvent the wheel + we standardise the algorithm to do it.

Just following some comments from Satoshi: https://bitcointalk.org/index.php?topic=423.msg3819#msg3819 and https://bitcointalk.org/index.php?topic=532.msg6306#msg6306.

Some considerations before implement 0-conf

In the big picture we need to:

• Broadcast the transaction to different nodes.
• Listen to many nodes a few seconds to check we don't receive any double-spend.

As far as I understand, this is could be an algorithm to process 0-conf payments, opened to changes, for sure:

1. Pick randomly from 4 to 8 nodes to work with.
2. Start listening from the nodes if there is any double-spend.
3. Broadcast the transaction to any node.
4. After 1 second, if we don't receive any double-spend we are safe.

Notice any of this parameters could be tuned, depending of the considerations of each wallet. If this is becoming standard, should be flexible enough.

Pending question: what to do in case we find a double-spend after we broadcast?

Bitcoinj-cash currently

As the original bitcoinj project works over Bitcoin Core, and it includes the RBF feature obviously it doesn't consider any of the previous points. Correct me if I'm wrong but when you include a broadcast transaction listener, it's only invoked if the transaction is accepted by a node, but you can't notice if any node rejects it. This node rejection message remains totally hidden, what makes the previous algorithm a little bit tricky. https://github.com/bitcoinj-cash/bitcoinj/blob/cash-0.14/core/src/main/java/org/bitcoinj/core/TransactionBroadcast.java#L91

Provide a solution to have all the required events over the transactions is a must to implement the a 0-conf algorithm.

Goals

• Clarify whether the control over the broadcast transaction responses suits with our needs.
• Implement a new TransactionBroadcaster https://github.com/bitcoinj-cash/bitcoinj/blob/cash-0.14/core/src/main/java/org/bitcoinj/core/TransactionBroadcaster.java that performs this new logic.
• Change the broadcastTransaction method to include a kind of listener to notify the response:

public TransactionBroadcast broadcastTransaction(Transaction tx, int minConnections)

public TransactionBroadcast broadcastTransaction(Transaction tx, int minConnections, BroadcastTransactionListener listener)

• Define the different scenarios we can face. This will define the different methods for the BroadcastTransactionListener. For instance:

 public interface BroadcastTransactionListener{

    void onBroadcastSuccess(Transaction tx);

    void onDoubleSpendReceived(Transaction broadcasted, Transaction received);

    void onProgress(int totalBroadcasts, int target);

}

What is your view for this? Many points to discuss, so please just let out any insight or comment you have.

Regards,

[HashEngineering]

This is a good start in considering 0-conf. Some RBF code is probably in bitcoinj and in our current bitcoinj-cash version. That could be removed as part of this project.

For 0-conf, there is the sender and the receiver. Both would want confidence in the transaction involved.

Some of your proposal is already part of bitcoinj-cash:

Pick randomly from 4 to 8 nodes to work with. Start listening from the nodes if there is any double-spend.

Currently as you pointed out, when broadcasting a transaction, the transactions are sent to about half the connected peers and it waits for the other half to respond with an INV message with the hash of the transaction. It also listens for rejected messages. Based on those two responses, it will indicate what will happen with the transaction.

Broadcast the transaction to any node. After 1 second, if we don't receive any double-spend we are safe.

These are not in bitcoinj. I do like your thinking in addressing the limitations.

For the Sender: If there are no double-spends or other rejections and other nodes respond with the inventory messages, then we are safe.

For the Receiver: The receiver will only get a inventory message if other nodes propagated the transaction.

Some nodes could reject the transaction while others don't based on the configurations of those nodes and other factors.

I will need to think on this further. My own app that uses bitcoinj-cash has some limitations in this matter also. It reports confirmations (up to 6 or 7 with a pie chart, visually). If the transaction is rejected by nodes (perhaps for fee or priority reasons) it will say that the Transaction hasn't been broadcast, but doesn't give a reason why. If the app receives inventory messages, then it will say that the transaction has been broadcast.

It will be interesting to see where this topic goes. Thanks @rseibane for creating this issue.

[rafa-js]

I'm starting to implement this 0-conf approach. I think it's better talk about the details with code some to express this ideas. I have in mind some ways to keep the details of the implementation very flexible and extensible. I will working on this branch. https://github.com/rseibane/bitcoinj/commits/0-conf

[HashEngineering]

I look forward to seeing what you come up with.

[rafa-js]

First commit. It compiles, but I haven't run the tests yet. https://github.com/rseibane/bitcoinj/commit/0adad629e15b0a68b0ea8cde0436d116715ac946

I've just cleanup the broadcast code:

• The files related to the transaction process has been moved from bitcoinj/core to bitcoinj/transaction. Here you can find the previous interfaces and classes and some more that I've divided in different files (before they were in the TransactionBroadcast).

• This method and its interface had inconsistent names, because what it does is to return the entity in charge of performing the broadcast (the TransactionBroadcaster), not broadcasting by themselves. I've renamed it to TransactionBroadcasterFactory and getTransactionBroadcaster.

• The current TransactionBroadcast class.

  1. TransactionBroadcaster describes better what it does and how it should be named.
  2. This can be generalised as an interface since we can create different ways to broadcast a transaction, so...
  3. The current TransactionBroadcast it's actually a concrete implementation of the new TransactionBroadcaster. Concretely the related with broadcast using a PeerGroup. So, I've renamed it to PeerGroupTransactionBroadcaster.

• I've identified two specific things that could be customised and provide a lot of flexibility.

  1. The way we choose the peers to perform the broadcast.
  2. The way we define the number of peers we want to achieve to consider the transaction broadcasted.
  3. This could be called BroadcastPeerGroupStrategy and be defined like this. And this is how the current strategy looks like.

This is the base for the next things to define. Now we have more flexibility to implement concrete parts and well defined concepts within the broadcast process.

Please, share your thoughts about this. Let me know if you find any suggestion or you find I'm breaking or forgetting something.

[HashEngineering]

This looks very interesting.

Some concerns are:

1. Current apps that use bitcoinj-cash may be broken by these changes. My own app doesn't use TransactionBroadcast -- perhaps this more of an internally used class so changes to it would be transparent to most developers and apps.
2. Merging with bitcoinj 0.15 - after checking the code for bitcoinj 0.15, one can see that segwit and bech32 addresses were added into the code a month ago, so merging will without the 0-conf feature will already be a big task and a separate topic.

At this point I don't see any problems with your ideas.

[rafa-js]

Hope to have a new commit at the end of the day. In the meaning while, I have a question about this code block: https://github.com/bitcoinj-cash/bitcoinj/blob/cash-0.14/core/src/main/java/org/bitcoinj/core/TransactionBroadcast.java#L99

• Why does it considers a transaction rejected if it gets a half of the target number of broadcasts? Should't it be rejecting if it gets just a single rejection?
• Are there any reasons makes a peer reject a transaction other than a double-spend?

Maybe Bitcoin Core and Bitcoin Cash required different answers...

[HashEngineering]

This is what we have for reasons for rejection. It includes other reasons by besides double-spend.

/** "reject" message codes */
static const uint8_t REJECT_MALFORMED = 0x01;
static const uint8_t REJECT_INVALID = 0x10;
static const uint8_t REJECT_OBSOLETE = 0x11;
static const uint8_t REJECT_DUPLICATE = 0x12;
static const uint8_t REJECT_NONSTANDARD = 0x40;
static const uint8_t REJECT_DUST = 0x41;
static const uint8_t REJECT_INSUFFICIENTFEE = 0x42;
static const uint8_t REJECT_CHECKPOINT = 0x43;

Some rejections would result from bugs in bitcoinj or the app that uses bitcoinj for not calculating outputs correctly (dust, insufficent fees) or forming the transaction incorrectly (nonstandard, invalid). Duplicatemight be double spend.

It is possible that a transaction could be rejected by one node and not another based on the configuration of the node. For instance if a BCH node was set up to require a higher fee, it would reject the transaction, but the other nodes would not.

If a new feature is added to transactions, which uses version 3, then older nodes would reject that transaction as non-standard. But upgraded nodes would not reject it. This may not be a big issue with Bitcoin Cash, which has hard forks and therefore older clients won't be on the network.

Given that there are multiple reasons for rejection and that nodes can return different results in some cases, that is proabably why bitcoinj current has it rule with half the target number of broadcasts. That doesn't mean that something cannot be improved.

Therefore it is necessary to see what multiple nodes return as rejection message and what the other nodes return with inventory messages. Perhaps for Double Spend, you might want to count it rejected if only 1 node returns that error.

Again, this is an overview. Specific reasons for each rejection message may give more detail and your implementation could be customized more.

[rafa-js]

This details are gold. Is there any place in the bitcoinj code where this codes are already defined?

[HashEngineering]

Yes, it is here in the RejectMessageclass.

https://github.com/bitcoinj-cash/bitcoinj/blob/cash-0.14/core/src/main/java/org/bitcoinj/core/RejectMessage.java

[rafa-js]

Thanks for the address.

More updates about this: https://github.com/rseibane/bitcoinj/commit/28b42f403b66a9f44b0cefd669149ef12743deb7

Basically I've included the double-spend checking and started to fit in all the components. Considerations for the double-spend detection:

• If my transaction is rejected because of REJECT_DUPLICATE I consider it's a double-spend.
• During the broadcast process I check if any of the transactions broadcasted in the network has a common input with my transaction. If it does, I consider is a double-spend. Not sure this is the correct way to go, please check this out. https://github.com/rseibane/bitcoinj/blob/0-conf/core/src/main/java/org/bitcoinj/broadcast/DoubleSpentDetector.java

This is how to broadcast a transaction would look like:

peerGroup.getTransactionBroadcaster(tx)
                .setBroadcastTransactionListener(new BroadcastTransactionListener() {
                    @Override
                    public void onBroadcastSuccess(Transaction tx, boolean isMined) {

                    }

                    @Override
                    public void onProgress(double percentage, int totalBroadcasts, int target) {

                    }

                    @Override
                    public void onDoubleSpendDetected(Transaction broadcastedTx, Transaction detectedTx) {

                    }

                    @Override
                    public void onBroadcastRejected(Transaction tx, RejectMessage rejectMessage) {

                    }
                })
                .broadcast();

Not sure if you can notice, but now work with futures doesn't look very cool. The reason is that the listener to get news about the broadcast is more complex than what a SettableFuture can do, and has been replaced by the BroadcastTransactionListener so the Future is no longer needed. I understand keeping it is good for other developers but I think this feature has big impact and requires all wallets be aware. I believe the integration would be very simple.

The next goal is to include a timer to stop listening to the network and consider the broadcast successful in case both the timer is elapsed and any double-spend is detected and we have reached as many peers as our target was set.

[rafa-js]

More updates: https://github.com/rseibane/bitcoinj/commit/39ae320a2929ce14170b25822b1834222631b50f

I have included the guard for a few seconds during the broadcast. Now to consider a transaction as successfully broadcasted both the timer has to elapse without detecting and any double-spend and we have to had reached as many peers as our target was set.

Additionally, I've included an example to run this, it seems to work fine: https://github.com/rseibane/bitcoinj/blob/0-conf/examples/src/main/java/org/bitcoinj/examples/SendRequestZeroConf.java

I've found a problem running some test, but unfortunately I cannot figure out the cause. This is the assert that fails: https://github.com/rseibane/bitcoinj/blob/0-conf/core/src/test/java/org/bitcoinj/core/PeerGroupTransactionBroadcasterTest.java#L255

Probably I will be off for a week or so. Hope to come back with new insights for the implementation.

[HashEngineering]

I will try to run the tests on your branch and figure out the cause of the test failures.

[HashEngineering]

There is a NullPointerException here:
peerGroup is null, but this call is made: peerGroup.getMinBroadcastConnections()

https://github.com/rseibane/bitcoinj/blob/39ae320a2929ce14170b25822b1834222631b50f/core/src/main/java/org/bitcoinj/broadcast/group/PeerGroupTransactionBroadcaster.java#L81

A quick fix is this: this.minConnections = peerGroup != null ? Math.max(1, peerGroup.getMinBroadcastConnections()) : 1;

But peerGroup will still be null. Perhaps there is a better way to do it like create a constructor specifically for the unit test that is not used by other parts of bitcoinj.

[rafa-js]

Got it, I understand the way to go. Great insights @HashEngineering I'll fix it and will report back the commit.

[rafa-js]

Fixed the null pointer https://github.com/rseibane/bitcoinj/commit/003fd302f80adaec39de3e9946f90b6056c0f121, but still the same assertion error:

java.lang.AssertionError: expected null, but was:<  181e5a610e28fb8ec60fba2f026eeb67c3cb613082f9a1b44c5974788f3d0129
     in   PUSHDATA(71)[304402204bf9edf85aca16c4b8837d9106561a5e33151f263ec8299448dc5fb9ad7074bb02202503b50ce6a7f9337a40746a5987540a491e7506dac61f457620df1317db6c3e01] PUSHDATA(33)[03fcb7a55ecf0c9c95fb2d729c15083080467c6e87caf92780568b3dbc59950b3b]
          outpoint:d5aed4affd539f2d140f82dc7b4540f22f66eaa65c6df2f1e7cbd3e90077f663:0
     out  DUP HASH160 PUSHDATA(20)[f7975aeb54ebafbc4a26c673dd796006f70068fa] EQUALVERIFY CHECKSIG 49.00 BCH
     out  DUP HASH160 PUSHDATA(20)[d789aed7d7f999ac12c0c523fbb3bfa921369a09] EQUALVERIFY CHECKSIG 1.00 BCH
     prps UNKNOWN
>

    at org.junit.Assert.fail(Assert.java:88)
    at org.junit.Assert.failNotNull(Assert.java:755)
    at org.junit.Assert.assertNull(Assert.java:737)
    at org.junit.Assert.assertNull(Assert.java:747)
    at org.bitcoinj.core.PeerGroupTransactionBroadcasterTest.peerGroupWalletIntegration(PeerGroupTransactionBroadcasterTest.java:255)

[rafa-js]

Finally it passes all the tests: https://github.com/rseibane/bitcoinj/commit/20674af502d17efc6563b4c21cd06baff4b6cdb4

Pending issues so far?

[rafa-js]

I've find out a weird scenario. During the broadcast process and waiting for the TransactionConfidence to change to know if has been accepted by the other peers, the confidence sometimes stops being updated. I've seen the log and it happens if the Wallet receives a pending transaction Wallet::receivePending before the targetNumSeenPeers is achieved.

Do you have any idea about this issue? What does the Wallet::receivePending call mean from a protocol point of view?

[HashEngineering]

Protocol View:

1. bitcoinj-cash will broadcast a newly created transaction to x peers and wait for y peers to reply with inv messages.
2. The transaction is propogated throughout the network. A node will broadcast an "inv" message to all its peers with the "tx" hash.
3. bitcoinj-cash will eventually receive some "inv" messages with the hash of the tx from step 1. It will then keep track of the peers that sent those messages and count them.

I don't see how receivePending is called under this scenario, unless the new transaction is not added to the pending list of the wallet in before step 1.

There are two cases where receivePending is called

1. an inv message is received with tx hashes that were not created by bitcoinj-cash
2. a transaction was created and broadcast successfully -- see PeerGroup.broadcastTransaction. receivePending is the method that adds a pending or non-confirmed transaction to the wallet to mark off spent coins (in the case of sending coins).

I am not sure about your issue. The second case of calling receivePending should be happening, but it sounds like perhaps the first case is happening. In the first case, no updates would be received until other peers announce the transaction, which may not be what you are talking about.

[rafa-js]

Thanks for replying so fast @HashEngineering

I don't see how receivePending is called under this scenario, unless the new transaction is not added to the pending list of the wallet in before step 1.

This is happening in my case, because I don't want to add it unless it has been broadcasted properly.

Anyway, I don't see the relation between the invocation to the Wallet::receivePending and the PeerGroupBroadcaster to stop receiving inv messages. I guess if both are related, one solution is to avoid the Wallet::receivePending. Are they? Is there any kind of message received from the peers that can force us to consider the broadcast as successful?

[HashEngineering]

I don't see how Wallet.receivePending is related to not receiving inv messages.

The success of a broadcast is measured by the number of inv messages received from the other nodes that the transaction was not sent to. I would need to try out your code with the example to see what is happening with it.

[rafa-js]

Now seems like it works. Not sure it was an issue with the code or with the network but the question is it didn't happen any more. I will report back if I can define the scenario much better.

Do you think the 0-conf should be always present to anyone who uses the library or just optional? If it was on me I would make it always present just as part of the library.

[HashEngineering]

I would say it should always be active.