Started patch for coinbase-tx appearance fix

[Jim Burton]

I have started the patch for fixing coinbase transactions appearing.

The core change is in the Transaction#isMine method in here:

http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/browse/core/src/main/java/com/google/bitcoin/core/Transaction.java

This method is not in the bitcoinj code and pulls in/ changes the following methods:

TransactionInput#isMine

Wallet#isTransactionRelevant

Wallet#processTxFromBestChain

Wallet#receive

The Wallet changes could probably be excluded for a narrow patch but they are all in there to fix bugs in MultiBit to do with tx appearance so I have incuded them.

I still have unit tests to write for the changes - will start looking at those tomorrow.

The Basic Fix

In the Transaction#isMine method any transactionInput that throws a ScriptException I log (at a low level as there are a lot of them) but do not throw a RuntimeException out the method.   I ignore them and give the boolean result of whether (for the scripts I understand) the transaction is mine.   This is the core of the fix really but it is hiding the fact that some scripts failed.

[Jim Burton]

For the coinbase-tx patch there is now code and a working test for a wallet accepting BTC from a coinbase transaction.  This is the basic required fix.

The test is CoinBaseBlockTest here:

https://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/browse/core/src/test/java/com/google/bitcoin/core/CoinBaseBlockTest.java

The test:

1) Creates a wallet with a private key I bought off a miner.

2) Gives it block 169482 which has some mining dues for that key.

3) Tests the balance after the block is processed is what it should be.

Question: still to do is making the coinbase tx unspendable. I believe in the protocol they are unspendable for 100 blocks but the Satoshi client has them unspendable for 120. For consistency with the Satoshi client do we want to make them unspendable for 120 blocks ?

(e.g a coinbase tx is received at block : 180,000.  It is still unspendable at 180,119 blocks. It is spendable at 180,120 blocks.)

Also to do:

1) I need to write a test for "a spend from a wallet to the same wallet". There is code in Wallet that I have added to do this.   It is a bit of a silly thing to do (it just burns a fee) but it is perfectly valid. The current released bitcoinj code misses this edge condition because the sendToMe and sendFromMe are both zero. It is a problem because the tx never confirms and so ties up your change.

2) All the existing tests are running cleanly except for one.   The test PeerTest#startBlockChainDownload fails when I run "mvn clean test" but runs ok when I run it in Eclipse.   Anyone seen anything similar ?

[Mike Hearn]

On Tue, May 1, 2012 at 6:48 PM, Jim Burton <jim...@fastmail.co.uk> wrote:

For the coinbase-tx patch there is now code and a working test for a wallet accepting BTC from a coinbase transaction.  This is the basic required fix.

The test is CoinBaseBlockTest here:

https://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/browse/core/src/test/java/com/google/bitcoin/core/CoinBaseBlockTest.java

Thanks! You could try putting the block in a file rather than in a huge string, I guess. Or create a fake block. I'm good with either approach. It's kind of weird to have a live production private key in the code. Is the miner sure they'll never use it again and there are no unspent coins on that key?

You should only need to provide the first transaction to the wallet. Providing all transactions shouldn't be harmful (I think) but the real code checks if a transaction is relevant before providing it to the wallet.

 

Question: still to do is making the coinbase tx unspendable. I believe in the protocol they are unspendable for 100 blocks but the Satoshi client has them unspendable for 120. For consistency with the Satoshi client do we want to make them unspendable for 120 blocks ?

Yes.

 

Also to do:

1) I need to write a test for "a spend from a wallet to the same wallet". There is code in Wallet that I have added to do this.   It is a bit of a silly thing to do (it just burns a fee) but it is perfectly valid. The current released bitcoinj code misses this edge condition because the sendToMe and sendFromMe are both zero. It is a problem because the tx never confirms and so ties up your change.

Are you sure? That's pretty much what happens for any TX with change and it seems to work?

 

2) All the existing tests are running cleanly except for one.   The test PeerTest#startBlockChainDownload fails when I run "mvn clean test" but runs ok when I run it in Eclipse.   Anyone seen anything similar ?

Unfortunately a few of the tests are flaky :( You can probably rerun it to see if it passes again.

[Jim]

Hi Mike,

Comments inline

On Wed, May 2, 2012, at 11:06 AM, Mike Hearn wrote:
> On Tue, May 1, 2012 at 6:48 PM, Jim Burton <jim...@fastmail.co.uk> wrote:
>
> > For the coinbase-tx patch there is now code and a working test for a
> > wallet accepting BTC from a coinbase transaction. This is the basic
> > required fix.
> >
> > The test is CoinBaseBlockTest here:
> >
> > https://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/browse/core/src/test/java/com/google/bitcoin/core/CoinBaseBlockTest.java
> >
>
> Thanks! You could try putting the block in a file rather than in a huge
> string, I guess. Or create a fake block. I'm good with either approach.
> It's kind of weird to have a live production private key in the code. Is
> the miner sure they'll never use it again and there are no unspent coins
> on
> that key?

I will put the block in a file and read it in.

RE: That private key - to fix a bug a miner raised (on BTC not
appearing)
I found it easiest to buy the key off him for the balance on it as then
I could process the actual coinbase transaction in all its glory.

First thing I did after fixing the BTC appearing in MultiBit was move
all the BTC off it to my own wallet !

As the test only deals with blockchain history it does not matter if
anyone uses that key for anything else - though it would be very unwise
as it is now in the public domain.

>
> You should only need to provide the first transaction to the wallet.
> Providing all transactions shouldn't be harmful (I think) but the real
> code
> checks if a transaction is relevant before providing it to the wallet.
>

I will strip it down to just the first tx. Quicker test.

>
> > *Question*: still to do is making the coinbase tx unspendable. I believe

> > in the protocol they are unspendable for 100 blocks but the Satoshi client
> > has them unspendable for 120. For consistency with the Satoshi client do we
> > want to make them unspendable for 120 blocks ?
> >
>
> Yes.

Will do.

>
>
> > Also to do:
> > 1) I need to write a test for "a spend from a wallet to the same wallet".
> > There is code in Wallet that I have added to do this. It is a bit of a
> > silly thing to do (it just burns a fee) but it is perfectly valid. The
> > current released bitcoinj code misses this edge condition because the
> > sendToMe and sendFromMe are both zero. It is a problem because the tx never
> > confirms and so ties up your change.
> >
>
> Are you sure? That's pretty much what happens for any TX with change and
> it
> seems to work?

The edge case is when both the recipient and sender are addresses in the
same
wallet so the net transfer of BTC for the tx is zero. There was some
code in
Wallet#isTransactionRelevant :

tx.getValueSentFromMe(this).compareTo(BigInteger.ZERO) > 0
|| tx.getValueSentToMe(this).compareTo(BigInteger.ZERO)
> 0

which drops the tx because the net is zero.

I only noticed it because one of the first things people do when trying
out MultiBit is do a send, and sometimes they would send it to
themselves.

The test to check for this has now been committed.

>
>
> > 2) All the existing tests are running cleanly except for one. The test
> > PeerTest#startBlockChainDownload fails when I run "mvn clean test" but runs
> > ok when I run it in Eclipse. Anyone seen anything similar ?
> >
>
> Unfortunately a few of the tests are flaky :( You can probably rerun it
> to
> see if it passes again.

Ok, will give it a go.
--
http://multibit.org Money, reinvented

[Mike Hearn]

The edge case is when both the recipient and sender are addresses in the

same wallet so the net transfer of BTC for the tx is zero.  

Ah, I see. Yeah, we should have a better definition of relevance. I haven't been happy with that code for a while now.

 

I only noticed it because one of the first things people do when trying
out MultiBit is do a send, and sometimes they would send it to
themselves.

You might want to consider Bitcoin Faucet integration. It's natural that people want to get started with a bit of money and the faucet is a good place to get some.

Please do submit pull requests or just plain diffs to me for these fixes when you feel ready.

[Jim]

Good idea on the Bitcoin Faucet - the first thing people want is some
BTC !

I am just about to do the "coinbase unavailable for 120 blocks".
Could you sketch out in a sentence or two what is required - I do not
want to miss anything and it will save time.

(Also, I want to do some "drilling and filling" on the boat this
afternoon
as the weather is nice!)

[Andreas Schildbach]

On 05/02/2012 11:58 AM, Mike Hearn wrote:

> You might want to consider Bitcoin Faucet integration. It's natural that
> people want to get started with a bit of money and the faucet is a good
> place to get some.

I investigated into this some months ago. IMHO what's missing is an URL
parameter to inject the own Bitcoin address. Fiddling around with cut &
paste is certainly not the kind of user experience people would expect.

Jim, let me know if you find anything out.

btw. Are there any alternatives to the faucet?

Cheers,

Andreas

[Mike Hearn]

Enjoy the good weather :-)

It's going to be fairly complicated I'm afraid. Several changes are needed which are independent. If you change your mind, I can work on this instead.

The first step is that the wallet needs to know its current best chain height/block. The reason will become clear later. This is already a TODO for us for some time now anyway. It means adding a new field to the wallet, the protobuf and making sure it's properly [de]serialized. Will need a few tests for this. It should be set in the receive() method when blockType == BEST_CHAIN, but don't set it redundantly, check first if this block has been seen before. It's totally valid for receive to be called with the same block more than once, but we want to have somewhere we can add some code later that is only run once per new best block.

This can be a standalone change you submit.

The next step is some improvements to the TransactionConfidence API. This can be a self contained change too. Firstly, we need to cache the depth in the best chain and, as you'll be in the general area, why not thrown in the amount of work done too. Currently the TransactionConfidence API looks like this:

    /**

     * Returns the chain height at which the transaction appeared if confidence type is BUILDING.

     * @throws IllegalStateException if the confidence type is not BUILDING.

     */

    public synchronized int getAppearedAtChainHeight();

    /**

     * Depth in the chain is an approximation of how much time has elapsed since the transaction has been confirmed. On

     * average there is supposed to be a new block every 10 minutes, but the actual rate may vary. The reference

     * (Satoshi) implementation considers a transaction impractical to reverse after 6 blocks, but as of EOY 2011 network

     * security is high enough that often only one block is considered enough even for high value transactions. For low

     * value transactions like songs, or other cheap items, no blocks at all may be necessary.<p>

     *     

     * If the transaction appears in the top block, the depth is one. If the transaction does not appear in the best

     * chain yet, throws IllegalStateException, so use {@link com.google.bitcoin.core.TransactionConfidence#getConfidenceType()}

     * to check first.

     *

     * @param chain a {@link BlockChain} instance.

     * @throws IllegalStateException if confidence type != BUILDING.

     * @return depth

     */

    public synchronized int getDepthInBlocks(BlockChain chain);

     /**

     * Returns the estimated amount of work (number of hashes performed) on this transaction. Work done is a measure of

     * security that is related to depth in blocks, but more predictable: the network will always attempt to produce six

     * blocks per hour by adjusting the difficulty target. So to know how much real computation effort is needed to

     * reverse a transaction, counting blocks is not enough.

     *

     * @param chain

     * @throws IllegalStateException if confidence type is not BUILDING

     * @return estimated number of hashes needed to reverse the transaction.

     */

    public synchronized BigInteger getWorkDone(BlockChain chain) throws BlockStoreException;

As you can see, to find out how deeply buried the tx is, in blocks or work, requires passing in a BlockChain and we then walk it backwards. This has two problems: (1) the wallet doesn't currently require the chain to create spends and it'd be nice to keep it that way, (2) it's slow. It means that every time you create a spend for a wallet that contains an unspent coinbase TX we'd have to walk large chunks of the chain. If you don't end up spending it for a while for some reason (ie, you are rich), spends will become slower and slower as we check and recheck that the depth is >120.

To fix it, the TransactionConfidence object needs to cache depth and work done inside itself. There's a subtlety here which is that a tx can appear in multiple divergent forks of the chain, each of which theoretically could have its own confidence. The code currently does not handle that - it simply throws exceptions in some cases when the TX is not on the best chain. That's kind of weak and makes the API less useful but doesn't need to be fixed right now. Just be aware of the difference between conceptual vs actual.

IMHO there should be a new method on this object:

/**

 * Called by the wallet when the tx appears on the best chain and a new block is added to the top. 

 * Updates the internal counters that track how deeply buried the block is.

 * Work is the value of block.getWork().

 * 

 * @throws IllegalStateException if state is not BUILDING

 */

public void notifyWorkDone(Block block) {

  Preconditions.checkState(getConfidenceType() == BUILDING);

  this.depth++;   // depth can just be an integer.

  this.workDone = this.workDone.add(block.getWork());

}

depth and workDone start out at zero. Calling notifyNotOnBestChain(), setAppearedAtChainHeight() should zero them out too. It means you can't get the cached depth in a general way for any chain fork which, as I said, is dumb, but we can fix later when we don't have to think about serialized wallets anymore.

In Wallet.receive, when the new block is on the best chain, after processing the transaction that's been received, we should check if this block hasn't been seen before (using the code from the first step). If this is a brand new block, go through every TX in the wallet except inactive/pending and call notifyWorkDone() on it with that block. Although this is O(N) in the size of the wallet, it's not very intensive so shouldn't matter in practice unless you have a huge wallet.

We must consider (and test!) the re-org case. It should "just work". When a TX moves between chains, it will either have notifyNotOnBestChain() called on it (which will zero out the counters and make methods that read them throw an exception due to the type not being BUILDING), or it will have setAppearedAtChainHeight() called on it, which will zero out the counters again. Then the replay process will call notifyWorkDone() as per normal so the depth counters should end up correct.

Now, we can obviously make TransactionConfidence.getDepthInBlocks() and TransactionConfidence.getWorkDone() just return the internal variables instead of their current implementations.

OK, so that's the second change. Now the wallet can find out the depth of any given transaction quickly, which will be useful for many other things later too.

Third change - in completeTx() we need to check a transaction is spendable. In the second loop here:

for (Transaction tx : unspent.values()) {

  ....

}

before scanning the outputs, we must check if a TX is spendable. I think here it's better to have the policy be outside of the wallet, in the TX itself maybe. For example we could have:

for (Transaction tx : unspent.values()) {

  if (!tx.isSpendable()) continue;

}

and Transaction.isSpendable() would be something like 

  if (isCoinbase()) 

    return getConfidence().getDepthInBlocks() > params.spendableCoinbaseDepth

  else

    return true;

In future this method could also check user provided policies, maybe you don't want to spend coins until they're 6 blocks deep, etc.

There is one more complexity to deal with I'm afraid. Coinbase transactions are abnormal because they do not survive re-orgs. This is the reason for the 120 block rule. Therefore we need to change the re-organize code to understand this. After we have done the "// Recalculate the unspent/spent buckets for the transactions the re-org did not affect." stage, we need to go through the onlyOldChainTransactions list and find the coinbase transactions, and ensure they end up being treated as dead. This of course also needs a test. Having the spendable coinbase depth on the params makes the tests faster and easier because we can set it to a smaller value, like 5 instead of 120.

That would be the third change. Phew!

[Mike Hearn]

+Gavin

Gavin maintains the Faucet. It's Python+AppEngine, adding a URL for the custom receive address should be a very simple change indeed. I CCd him so he can comment. There may also need to be a bit of HTML work to make it look nice on a small screen, not sure.

The harder part is the need for a Google account, but all your users should already have one. Recent versions of Android will sign you in via the web automatically so hopefully there's little friction there. Older Androids may suck more in this regard.

Alternatives - there's a site that lets you buy Bitcoins by SMS, so you pay via your phone bill, but they charge a huge markup. It's fine for playing around.

[Jim]

Hi Mike,

Thanks for that level of detail.
I think I will print that email out and go through it a few times
with a strong cup of coffee!

The change to add to the wallet the current best chain height/ block
is something on my list of TODOs also - it makes it able to close
a wallet in the MultiBit UI and reopen it and then catch up the
blockchain. It makes wallets on (normally detached) USBs doable.

I will work through your outline and do patches for each chunk
of work in turn to keep it manageable. Then the current fork
I have which makes coinbase txes start to appear can be merged in
safely.

I am quite happy to work through it as most of the coming
MultiBit stuff (wallets upgrades, encryption etc) straddle
the two code bases so it should go into bitcoinj and not
MultiBit.

Cheers,

Jim

> have its own confidence. The code currently does *not* handle that - it

[Andreas Schildbach]

On 05/02/2012 02:05 PM, Mike Hearn wrote:

> Gavin maintains the Faucet. It's Python+AppEngine, adding a URL for the
> custom receive address should be a very simple change indeed. I CCd him
> so he can comment. There may also need to be a bit of HTML work to make
> it look nice on a small screen, not sure.

Yes, a smallscreen version would be great. But that has time for later.

> The harder part is the need for a Google account, but all your users
> should already have one. Recent versions of Android will sign you in via
> the web automatically so hopefully there's little friction there. Older
> Androids may suck more in this regard.

Android 3+ signs in automatically; the others will benefit from the
browser password cache (if the user has used his Google login in the
browser before).

Even typing in the account is not that bad, compared to typing the
Bitcoin address.

Cheers,

Andreas

[Andreas Schildbach]

I think the main thing we'd need right now is the ability to supply a
destination bitcoin address via URL parameter (maybe this is already
possible right now, in this case it should be documented). The address
should be remembered through the login/captcha/validation process.

That way, any app (be it Android, iOS or Desktop) could open the browser
with that URL containing the destination bitcoin address. No JavaScript
involved.


On 05/02/2012 03:40 PM, Gavin Andresen wrote:
> I'd be happy to make the Faucet easier to use for Android users. I'm
> not sure how a "URL parameter to inject their own Bitcoin address"
> would work, though (I know nothing about HTML/browsers on mobile
> phones, are there magic JavaScript dictionaries that apps can use to
> make information like "my bitcoin address" available?).
>

[Andreas Schildbach]

Thanks Mike for this elaborate post!

> The first step is that the wallet needs to know its current best chain
> height/block.

This is issue http://code.google.com/p/bitcoinj/issues/detail?id=121

It would also be useful for detecting cases where the blockchain has
become out of sync with a wallet (for example because of a crash).

> Third change - in completeTx() we need to check a transaction is
> spendable.

A question about Wallet.getBalance(). I guess coinbase will be included
in BalanceType.AVAILABLE only if it is spendable, that means after
100/120 blocks have passed.

But what about BalanceType.ESTIMATED? Would you include it there, right
from the start?

Cheers,

Andreas

[Mike Hearn]

A question about Wallet.getBalance(). I guess coinbase will be included
in BalanceType.AVAILABLE only if it is spendable, that means after
100/120 blocks have passed.

But what about BalanceType.ESTIMATED? Would you include it there, right
from the start?

Good question. The whole AVAILABLE/ESTIMATED thing needs to die at some point anyway. It's a holdover from the times when we didn't support pending transactions very well. The "must be in at least one block to spend" rule could just go away, I think, subject to a few caveats. It's on my never-ending to do list :-)

[Jim]

Hi Mike,

I have done the first phase of the coinbase-tx appearance fix.
(the bit to do with storing the lastSeenBlockHash - see email
excerpt at end of email).

It is my bitcoinj clone:

jimburton618-bitcoinj-coinbase-tx

in the branch:
coinbase-phase1

I could not see a way in the https://code.google.com UI
to do a pull request. The changes should be based on the
bitcoinj master branch if my git usage is right.

Let me know if there is anything you spot that needs
redoing. There are new tests in WalletTest and
WalletProtobufSerializerTest.

I will probably start the next changes (TransactionConfidence)
in a 'phase2' branch over the weekend.

Jim

-----------------------------------------------------

On Wed, May 2, 2012, at 01:50 PM, Mike Hearn wrote:
>

> The first step is that the wallet needs to know its current best chain
> height/block. The reason will become clear later. This is already a TODO
> for us for some time now anyway. It means adding a new field to the
> wallet,
> the protobuf and making sure it's properly [de]serialized. Will need a
> few
> tests for this. It should be set in the receive() method when blockType
> ==
> BEST_CHAIN, but don't set it redundantly, check first if this block has
> been seen before. It's totally valid for receive to be called with the
> same
> block more than once, but we want to have somewhere we can add some code
> later that is only run once per new best block.
>
> This can be a standalone change you submit.

------------------------------------------------------

[Miron Cuperman]

Jim,

Could you rebase on the latest master and collapse to fewer commits (or one)?  It's hard to see what are the final changes with all the incremental work.

You can rebase onto master like this:

    git checkout -b new-branch-1

    git fetch upstream

    git rebase upstream/master

assuming "upstream" is the main bitcoinj remote in your repository.

Then you can squash commits with:

  git rebase -i upstream/master

which will allow you to interactively choose commits to squash.

[Jim]

Miron,

Yes, will do.

Jim

[Mike Hearn]

Yep, thanks. I'll review it once this is done. Looking forward to it!

--

Mike Hearn |

Senior Software Engineer |

he...@google.com |

Account security team

[Jim Burton]

I have rebased the first set of changes for the coinbase-tx patch (add lastSeenBlockHash to wallet and protobuf).

The commit is 1a00bd6375b3 on the coinbase-phase1-rebase branch in my clone jimburton618-bitcoinj-coinbase-tx .

The list of changes is here:

http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=1a00bd6375b3af6a0ade02bd38478cedc3179827&name=coinbase-phase1-rebase

I think I have done it right but let me know if there is any mistakes in there.

Jim

[Mike Hearn]

Thanks! It's looking pretty good.

Just a few more minor changes please:

1. Some of the changed files add imports and nothing else - maybe set your IDE to delete unused imports on save?
   
   
2. Some of the changes are just formatting changes. Maybe use your IDEs diff viewer to revert those chunks leaving only the core of the change?
   
   
3. Some of the new code doesn't indent the same way as the existing code:
   
   http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/diff?show=review&format=side&path=/core/src/main/java/com/google/bitcoin/core/Wallet.java&r=1a00bd6375b3af6a0ade02bd38478cedc3179827&spec=svn1a00bd6375b3af6a0ade02bd38478cedc3179827
   
   is an example of that (at the bottom). We use 4 space indents.
   
   
4. You can use the WalletProtobufSerializer.hashToByteString method to convert a Sha256Hash to a ByteString - no need to do it by hand. There is also byteStringToHash to go the other way. It reduces a bit of duplication in that part of the code.
   
   
5. Protobufs provide a .hasFoo() method for optional fields - checking this is better than comparing against the empty string.
   
   
6. I like the new test utility! For extra realism could you round trip the transactions via serialized form? Just use bitcoinSerialize and then recreate the objects - this way we ensure any internal state is cleared and it's as if we just read off the wire. In the past there have been subtle bugs caused by not doing this.
   
   
7. In the rememberLastBlockSeenHash() test, the fake blocks are created using the block store. By default this means they'll all chain together. It's not critical (I think the test will still work correctly), but it's a bit confusing as you are sending b2 to the wallet as a side chain. You can just add a TODO with my name on it here and I'll fix this after merging.
   
   

[Jim]

Hi Mike,

I will work through your points tomorrow (Wed) and create another commit
with them all in.

Cheers

On Tue, May 15, 2012, at 10:13 PM, Mike Hearn wrote:
> Thanks! It's looking pretty good.
>
> Just a few more minor changes please:
>

> 1. Some of the changed files add imports and nothing else - maybe set

> your IDE to delete unused imports on save?
>

> 2. Some of the changes are just formatting changes. Maybe use your

> IDEs
> diff viewer to revert those chunks leaving only the core of the
> change?
>

> 3. Some of the new code doesn't indent the same way as the existing

> code:
>
>
> http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/diff?show=review&format=side&path=/core/src/main/java/com/google/bitcoin/core/Wallet.java&r=1a00bd6375b3af6a0ade02bd38478cedc3179827&spec=svn1a00bd6375b3af6a0ade02bd38478cedc3179827
>
> is an example of that (at the bottom). We use 4 space indents.
>

> 4. You can use the WalletProtobufSerializer.hashToByteString method to

> convert a Sha256Hash to a ByteString - no need to do it by hand. There
> is
> also byteStringToHash to go the other way. It reduces a bit of
> duplication
> in that part of the code.
>

> 5. Protobufs provide a .hasFoo() method for optional fields - checking

> this is better than comparing against the empty string.
>

> 6. I like the new test utility! For extra realism could you round trip

> the transactions via serialized form? Just use bitcoinSerialize and
> then
> recreate the objects - this way we ensure any internal state is
> cleared and
> it's as if we just read off the wire. In the past there have been
> subtle
> bugs caused by not doing this.
>

> 7. In the rememberLastBlockSeenHash() test, the fake blocks are

> created
> using the block store. By default this means they'll all chain
> together.
> It's not critical (I think the test will still work correctly), but
> it's a
> bit confusing as you are sending b2 to the wallet as a side chain. You
> can
> just add a TODO with my name on it here and I'll fix this after
> merging.
>
>
> On Tue, May 15, 2012 at 2:39 PM, Jim Burton <jim...@fastmail.co.uk>
> wrote:
>
> > I have rebased the first set of changes for the coinbase-tx patch (add
> > lastSeenBlockHash to wallet and protobuf).
> >

> > The commit is *1a00bd6375b3 *on the coinbase-phase1-rebase branch in my

> >>>> jimburton618-bitcoinj-**coinbase-tx

> >>>>
> >>>> in the branch:
> >>>> coinbase-phase1
> >>>>
> >>>> I could not see a way in the https://code.google.com UI
> >>>> to do a pull request. The changes should be based on the
> >>>> bitcoinj master branch if my git usage is right.
> >>>>
> >>>> Let me know if there is anything you spot that needs
> >>>> redoing. There are new tests in WalletTest and
> >>>> WalletProtobufSerializerTest.
> >>>>
> >>>> I will probably start the next changes (TransactionConfidence)
> >>>> in a 'phase2' branch over the weekend.
> >>>>
> >>>> Jim
> >>>>

> >>>> ------------------------------**-----------------------

> >>>>
> >>>> On Wed, May 2, 2012, at 01:50 PM, Mike Hearn wrote:
> >>>> >
> >>>> > The first step is that the wallet needs to know its current best chain
> >>>> > height/block. The reason will become clear later. This is already a
> >>>> TODO
> >>>> > for us for some time now anyway. It means adding a new field to the
> >>>> > wallet,
> >>>> > the protobuf and making sure it's properly [de]serialized. Will need a
> >>>> > few
> >>>> > tests for this. It should be set in the receive() method when
> >>>> blockType
> >>>> > ==
> >>>> > BEST_CHAIN, but don't set it redundantly, check first if this block
> >>>> has
> >>>> > been seen before. It's totally valid for receive to be called with the
> >>>> > same
> >>>> > block more than once, but we want to have somewhere we can add some
> >>>> code
> >>>> > later that is only run once per new best block.
> >>>> >
> >>>> > This can be a standalone change you submit.
> >>>>

> >>>> ------------------------------**------------------------

[Jim Burton]

I have updated the coinbase-tx phase1 commit with Mike's comments from yesterday. I think I have sorted everything.

The new commit is:

92ec7f8e3ea8

It is here:

http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=92ec7f8e3ea8ab037715806792436a831953d4cf&name=coinbase-phase1-rebase

The class TransactionInput is in the list of patched files but there is absolutely no change from the master. I think it is due to it having imports added in one commit, and then removed. I don't think it is important.

In the TestUtils I took the liberty of roundtripping the tx that the original createFakeTx method created in addition to the method I added for consistency.

The tests all run ok.

[Andreas Schildbach]

The patch still changes one file without need (TransactionInput.java)
and there are some badly formatted lines (whitespace at end, will be
changed again with next code format).

I'd recommend examining your patches with "git show <hash>". Whitespace
at eol will be marked in red.

On 05/16/2012 06:30 PM, Jim Burton wrote:
> I have updated the coinbase-tx phase1 commit with Mike's comments from
> yesterday. I think I have sorted everything.
>
> The new commit is:

> *92ec7f8e3ea8*
> *
> *

> <mailto:jim...@fastmail.co.uk>>

> <mailto:he...@google.com> | Account

> > >> security team
> > >>
> > >
> >
> >
> > --
> >
> > Mike Hearn | Senior Software Engineer | he...@google.com

> <mailto:he...@google.com> | Account

[Jim Burton]

I have gone through the coinbase-tx phase1 changes fixing the whitespace.

There is a new commit:

6c31abd698ae

When you do a "git show 6c31abd698ae" there are just the real changes.

TransactionInput does not appear in the patch list now.

To look at it online it is here:

http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=6c31abd698aea42dcf0442ec63ff2e61a2847f2b&name=coinbase-phase1-rebase#

The tests all pass.

[Andreas Schildbach]

This looks good to me now.

On 05/17/2012 03:32 PM, Jim Burton wrote:
> I have gone through the coinbase-tx phase1 changes fixing the whitespace.
> There is a new commit:
>

> *6c31abd698ae*
> *
> *

> > <mailto:jim...@fastmail.co.uk <mailto:jim...@fastmail.co.uk>>>

> > > wrote:
> > >
> > > > I have rebased the first set of changes for the coinbase-tx
> > patch (add
> > > > lastSeenBlockHash to wallet and protobuf).
> > > >
> > > > The commit is *1a00bd6375b3 *on the coinbase-phase1-rebase
> > branch in my
> > > > clone jimburton618-bitcoinj-coinbase-tx .
> > > >
> > > > The list of changes is here:
> > > >
> > > >
> >
> http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=1a00bd6375b3af6a0ade02bd38478cedc3179827&name=coinbase-phase1-rebase
> <http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=1a00bd6375b3af6a0ade02bd38478cedc3179827&name=coinbase-phase1-rebase>
>
> >
> <http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=1a00bd6375b3af6a0ade02bd38478cedc3179827&name=coinbase-phase1-rebase
> <http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=1a00bd6375b3af6a0ade02bd38478cedc3179827&name=coinbase-phase1-rebase>>
>
> >
> > > >
> > > > I think I have done it right but let me know if there is any
> > mistakes in
> > > > there.
> > > >
> > > > Jim
> > > >
> > > >
> > > > On Friday, May 11, 2012 10:43:25 PM UTC+1, Mike Hearn wrote:
> > > >>
> > > >> Yep, thanks. I'll review it once this is done. Looking
> forward
> > to it!
> > > >>
> > > >> On Fri, May 11, 2012 at 1:38 PM, Miron Cuperman
> > <mi...@google.com <mailto:mi...@google.com>

> <mailto:mi...@google.com <mailto:mi...@google.com>>> wrote:
> > > >>
> > > >>> Jim,
> > > >>>
> > > >>> Could you rebase on the latest master and collapse to fewer
> > commits (or
> > > >>> one)? It's hard to see what are the final changes with all
> > the incremental
> > > >>> work.
> > > >>>
> > > >>> You can rebase onto master like this:
> > > >>>
> > > >>> git checkout -b new-branch-1
> > > >>> git fetch upstream
> > > >>> git rebase upstream/master
> > > >>>
> > > >>> assuming "upstream" is the main bitcoinj remote in your
> > repository.
> > > >>>
> > > >>> Then you can squash commits with:
> > > >>>
> > > >>> git rebase -i upstream/master
> > > >>>
> > > >>> which will allow you to interactively choose commits to
> squash.
> > > >>>
> > > >>>
> > > >>> On Fri, May 11, 2012 at 12:45 PM, Jim
> <jim...@fastmail.co.uk <mailto:jim...@fastmail.co.uk>

> > <mailto:jim...@fastmail.co.uk <mailto:jim...@fastmail.co.uk>>>

> > <mailto:he...@google.com <mailto:he...@google.com>> | Account

> > > >> security team
> > > >>
> > > >
> > >
> > >
> > > --
> > >
> > > Mike Hearn | Senior Software Engineer | he...@google.com
> <mailto:he...@google.com>

> > <mailto:he...@google.com <mailto:he...@google.com>> | Account

[Mike Hearn]

Thanks for being so patient Jim! I agree this looks good!

Miron noted that the Protos.java file changed in unexpected ways. I'll
just revert that file after merging.

I have to do some Gmail upgrades today, I thought I'd get some time to
do bitcoin this week but forgot it was my shift. I'll try and merge
this ASAP.

Mike Hearn | Senior Software Engineer | he...@google.com | Account security team

[Mike Hearn]

Miron noted that the Protos.java file changed in unexpected ways. I'll
just revert that file after merging.

Oops, just saw the other thread. The commit is fine as is and I'll pull once I completed my other day-job tasks.

[Jim]

Great. I will get on with the TransactionConfidence change. 

jim...@fastmail.co.uk

[Mike Hearn]

Pulled. Thanks again!

[Jim Burton]

I have completed the changes to get TransactionConfidence to store internally the depth and workDone.

The purpose of this is so that wallets with coinbase transactions do not have to traverse 100+ blocks just to see if they are spendable.

The commit is here:

http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=3b507115ccf1ade6423cce21a1025159bedd5192&name=coinbase-phase2

(commit: 3b507115ccf1) 

It is rebased against the master (0.6-SNAPSHOT) and squashed.

The tests all run ok.

When I ran maven on the command line it did not seem to pick up ChainSplitTests as a JUnit so have renamed it to ChainSplitTest. I think there must be a "*Test" classname filter somewhere. This makes it a bit hard to see what I have changed: I have just beefed up the txConfidenceLevels test to check depth and workDone work with reorgs. The best and side chains used are slightly longer too.

Let me know what you think, especially on the reorg code, in case I have missed something.

On Friday, May 18, 2012 1:36:05 PM UTC+1, Mike Hearn wrote:

Pulled. Thanks again!

[Andreas Schildbach]

On 05/28/2012 03:03 PM, Jim Burton wrote:

> When I ran maven on the command line it did not seem to pick up

> ChainSplitTest*s *as a JUnit so have renamed it to ChainSplitTest. I

> think there must be a "*Test" classname filter somewhere. This makes it
> a bit hard to see what I have changed

This should probably fixed in a separate commit on the 0.6/0.5 branches,
and then if you rebase against that your changes will be more obvious.

Cheers,

Andreas

[Jim]

The google code web UI does a good job of doing a colour coded diff
between the added ChainSplitTest and deleted ChainSplitTests.

If you look at:
http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=3b507115ccf1ade6423cce21a1025159bedd5192&name=coinbase-phase2

and then expand the ChainSplitTest node you see the diff.

[Mike Hearn]

This is looking really great. I posted a review on the commit, this time none of them are style related, it's all pure functionality. The code looks exactly how I'd imagined writing it myself, so I'm confident we can get this merged really fast.

[Mike Hearn]

One thing that should probably be done as part of this patch - store the cached values in the wallet.

[Jim Burton]

Mike <- Just read your comment about persisting the workDone and depth. Yes will do.

Here is the mail I was just writing about your comments:

( I will do another patch with the persistence changes so this is just to see if you are

happy with the fiixes).

I have gone through Mike's comments and (hopefully) addressed them all as follows:

/core/src/main/java/com/google/bitcoin/core/TransactionConfidence.java   r3b507115ccf1ade6423cce21a1025159bedd5192   line 347:

347:     public synchronized void setWorkDone(BigInteger workDone) {

This one is public but the setDepthInBlocks is package private - intentional?

Jim: I have changed the setter to public to match the getter.

/core/src/main/java/com/google/bitcoin/core/Wallet.java   r3b507115ccf1ade6423cce21a1025159bedd5192   line 569:

569:         if (bestChain) {

Hmm, this happens AFTER invokeOnTransactionConfidenceChanged - shouldn't we try and keep the event listener being run after all the changes are made?

Jim: The invokeOnTransactionConfidenceChanged is now after the "if (bestChain)" statement. Note that the tx.setBlockAppearance is still before the "if (bestChain)"

/core/src/main/java/com/google/bitcoin/core/Wallet.java   r3b507115ccf1ade6423cce21a1025159bedd5192   line 1502:

1502:         int depthToSubtract = 0;

You could just initialize this to the size of oldBlocks

Jim: Done.

/core/src/main/java/com/google/bitcoin/core/Wallet.java   r3b507115ccf1ade6423cce21a1025159bedd5192   line 1539:

1539:             // If there are no new transactions in this block we still need to bury existing transactions one block deeper

Good, but how about putting the for loop into the else branch of this condition. Full stop at the end of this comment.

Jim: Done.

/core/src/main/java/com/google/bitcoin/core/Wallet.java   r3b507115ccf1ade6423cce21a1025159bedd5192   line 1594:

1594:     private void subtractDepthAndWorkDone(int depthToSubtract, BigInteger workDoneToSubtract, Collection<Transaction> transactions) {

Probably should be synchronized

Jim: Done.

/core/src/main/java/com/google/bitcoin/core/Wallet.java   r3b507115ccf1ade6423cce21a1025159bedd5192   line 1607:

1607:     private void incrementDepthAndWorkDone(StoredBlock newBlock, Collection<Transaction> transactions) throws VerificationException {

Is this the same as

for (Transaction tx : transactions)  tx.notifyWorkDone(block);

?

In which case it may be simpler to not have a dedicated method for this and just reuse notifyWorkDone from inside reorganize()

Jim: It is *almost* the same. The only difference is that notifyWorkDone previously had a Precondition to check for the tx being in the confidence state of 'building' whereas the method filtered them out. Looking at the code all the calls to notifyWorkDone were filtering for 'building' tx so I have put the 'if' in notifyWorkDone and removed the Precondition. (It is hence slightly more 'liberal'). Tx in the state 'OVERRIDDEN_BY_DOUBLE_SPEND' were certainly being passed to notifyWorkDone in one of the ChainSplitTest tests so the behaviour is different. 

The commit is  bbeed1c0c09a and the changeset is here:

http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=bbeed1c0c09af5fee91d646bc2fcecf9ba684bc7&name=coinbase-phase2

[Mike Hearn]

This looks good to me. You can do the wallet persistence in a separate commit if you like. Once that's done I'll merge this. It doesn't make sense to merge before persistence is done, I think.

[Jim Burton]

There is a commit for the depth and workDone persistence in the wallet here:

http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=1ff337a8f6903ad3af5979c6f729bae50ffcca43&name=coinbase-phase2#

Commit: 1ff337a8f690

This is in addition to the previous commit: 39f06ed200d6

There is an extra test in WalletProtobufSerializerTest to check that they roundtrip ok.

:-)

[Mike Hearn]

LGTM, though it's weird that getWorkDone() can return NULL! I didn't notice that before.

The sorting transactions in depth order thing feels like it could be a useful method rather than a one-off for the case of two transactions. Sorting stuff is pretty easy with Collections.sort(). But it's not a big deal.

I'll merge now.

[Mike Hearn]

Done. I squashed the two commits together and made some merge fixes, so it's now (from gits perspective) a different commit. Hope that isn't too inconvenient.

Next step should be easier  - make sure coinbase transactions behave properly across deep re-orgs. Keep going! :)

[Jim]

Great,
I'll start on the last bit to see if outputs are spendable and coinbase
tx in reorgs over the weekend.

On Fri, Jun 1, 2012, at 04:16 PM, Mike Hearn wrote:
> Done. I squashed the two commits together and made some merge fixes, so
> it's now (from gits perspective) a different commit. Hope that isn't too
> inconvenient.
>
> Next step should be easier - make sure coinbase transactions behave
> properly across deep re-orgs. Keep going! :)
>
> On Fri, Jun 1, 2012 at 4:04 PM, Mike Hearn <he...@google.com> wrote:
>
> > LGTM, though it's weird that getWorkDone() can return NULL! I didn't
> > notice that before.
> >
> > The sorting transactions in depth order thing feels like it could be a
> > useful method rather than a one-off for the case of two transactions.
> > Sorting stuff is pretty easy with Collections.sort(). But it's not a big
> > deal.
> >
> > I'll merge now.
> >
> >
> > On Fri, Jun 1, 2012 at 2:25 PM, Jim Burton <jim...@fastmail.co.uk> wrote:
> >
> >> There is a commit for the depth and workDone persistence in the wallet
> >> here:
> >>
> >>
> >> http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=1ff337a8f6903ad3af5979c6f729bae50ffcca43&name=coinbase-phase2#
> >>

> >> Commit: *1ff337a8f690*
> >> This is in addition to the previous commit: *39f06ed200d6*

> >>
> >> There is an extra test in WalletProtobufSerializerTest to check that they
> >> roundtrip ok.
> >>
> >> :-)
> >>
> >>
> >> On Wednesday, May 30, 2012 9:25:48 PM UTC+1, Mike Hearn wrote:
> >>>
> >>> This looks good to me. You can do the wallet persistence in a separate
> >>> commit if you like. Once that's done I'll merge this. It doesn't make sense
> >>> to merge before persistence is done, I think.
> >>>
> >>> On Wed, May 30, 2012 at 7:07 PM, Jim Burton <jim...@fastmail.co.uk>wrote:
> >>>
> >>>> Mike <- Just read your comment about persisting the workDone and depth.
> >>>> Yes will do.
> >>>>
> >>>> Here is the mail I was just writing about your comments:
> >>>> ( I will do another patch with the persistence changes so this is just
> >>>> to see if you are
> >>>> happy with the fiixes).
> >>>>
> >>>>
> >>>> I have gone through Mike's comments and (hopefully) addressed them all
> >>>> as follows:
> >>>>

> >>>> /core/src/main/java/com/**google/bitcoin/core/**TransactionConfidence.java
> >>>> r3b507115ccf1ade6423cce21a1025**159bedd5192 line 347<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/browse/core/src/main/java/com/google/bitcoin/core/TransactionConfidence.java?spec=svn3b507115ccf1ade6423cce21a1025159bedd5192&r=3b507115ccf1ade6423cce21a1025159bedd5192#347>
> >>>> :
> >>>>
> >>>>
> >>>> 347: * public synchronized void setWorkDone(BigInteger workDone) {*

> >>>>
> >>>>
> >>>> This one is public but the setDepthInBlocks is package private - intentional?
> >>>>

> >>>> *Jim: *I have changed the setter to public to match the getter.
> >>>>
> >>>> /core/src/main/java/com/**google/bitcoin/core/Wallet.**java
> >>>> r3b507115ccf1ade6423cce21a1025**159bedd5192 line 569<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/browse/core/src/main/java/com/google/bitcoin/core/Wallet.java?spec=svn3b507115ccf1ade6423cce21a1025159bedd5192&r=3b507115ccf1ade6423cce21a1025159bedd5192#569>
> >>>> :
> >>>>
> >>>>
> >>>> 569: * if (bestChain) {*
> >>>>
> >>>>
> >>>> Hmm, this happens AFTER invokeOnTransactionConfidenceC**hanged - shouldn't we try and keep the event listener being run after all the changes are made?
> >>>>
> >>>> *Jim: *The invokeOnTransactionConfidenceC**hanged is now after the "if (bestChain)" statement. Note that the tx.setBlockAppearance is still before the "if (bestChain)"
> >>>>
> >>>> /core/src/main/java/com/**google/bitcoin/core/Wallet.**java
> >>>> r3b507115ccf1ade6423cce21a1025**159bedd5192 line 1502<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/browse/core/src/main/java/com/google/bitcoin/core/Wallet.java?spec=svn3b507115ccf1ade6423cce21a1025159bedd5192&r=3b507115ccf1ade6423cce21a1025159bedd5192#1502>
> >>>> :
> >>>>
> >>>>
> >>>> 1502: * int depthToSubtract = 0;*

> >>>>
> >>>>
> >>>> You could just initialize this to the size of oldBlocks
> >>>>

> >>>> *Jim: *Done.
> >>>>
> >>>> /core/src/main/java/com/**google/bitcoin/core/Wallet.**java
> >>>> r3b507115ccf1ade6423cce21a1025**159bedd5192 line 1539<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/browse/core/src/main/java/com/google/bitcoin/core/Wallet.java?spec=svn3b507115ccf1ade6423cce21a1025159bedd5192&r=3b507115ccf1ade6423cce21a1025159bedd5192#1539>
> >>>> :
> >>>>
> >>>>
> >>>> 1539: * // If there are no new transactions in this block we still need to bury existing transactions one block deeper*

> >>>>
> >>>>
> >>>> Good, but how about putting the for loop into the else branch of this condition. Full stop at the end of this comment.
> >>>>

> >>>> *Jim: *Done.
> >>>>
> >>>> /core/src/main/java/com/**google/bitcoin/core/Wallet.**java
> >>>> r3b507115ccf1ade6423cce21a1025**159bedd5192 line 1594<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/browse/core/src/main/java/com/google/bitcoin/core/Wallet.java?spec=svn3b507115ccf1ade6423cce21a1025159bedd5192&r=3b507115ccf1ade6423cce21a1025159bedd5192#1594>
> >>>> :
> >>>>
> >>>>
> >>>> 1594: * private void subtractDepthAndWorkDone(int depthToSubtract, BigInteger workDoneToSubtract, Collection<Transaction> transactions) {*
> >>>>
> >>>>
> >>>> Probably should be synchronized
> >>>>
> >>>> *Jim: *Done.
> >>>>
> >>>> /core/src/main/java/com/**google/bitcoin/core/Wallet.**java
> >>>> r3b507115ccf1ade6423cce21a1025**159bedd5192 line 1607<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/browse/core/src/main/java/com/google/bitcoin/core/Wallet.java?spec=svn3b507115ccf1ade6423cce21a1025159bedd5192&r=3b507115ccf1ade6423cce21a1025159bedd5192#1607>
> >>>> :
> >>>>
> >>>>
> >>>> 1607: * private void incrementDepthAndWorkDone(StoredBlock newBlock, Collection<Transaction> transactions) throws VerificationException {*

> >>>>
> >>>>
> >>>> Is this the same as
> >>>>
> >>>> for (Transaction tx : transactions) tx.notifyWorkDone(block);
> >>>>
> >>>> ?
> >>>>
> >>>> In which case it may be simpler to not have a dedicated method for this and just reuse notifyWorkDone from inside reorganize()
> >>>>
> >>>> Jim: It is *almost* the same. The only difference is that notifyWorkDone previously had a Precondition to check for the tx being in the confidence state of 'building' whereas the method filtered them out. Looking at the code all the calls to notifyWorkDone were filtering for 'building' tx so I have put the 'if' in notifyWorkDone and removed the Precondition. (It is hence slightly more 'liberal'). Tx in the state 'OVERRIDDEN_BY_DOUBLE_SPEND' were certainly being passed to notifyWorkDone in one of the ChainSplitTest tests so the behaviour is different.
> >>>>
> >>>>

> >>>> The commit is *bbeed1c0c09a *and the changeset is here:
> >>>> http://code.google.com/r/**jimburton618-bitcoinj-**
> >>>> coinbase-tx/source/detail?r=**bbeed1c0c09af5fee91d646bc2fcec**
> >>>> f9ba684bc7&name=coinbase-**phase2<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=bbeed1c0c09af5fee91d646bc2fcecf9ba684bc7&name=coinbase-phase2>

> >>>>
> >>>>
> >>>>
> >>>> On Tuesday, May 29, 2012 4:58:13 PM UTC+1, Mike Hearn wrote:
> >>>>>
> >>>>> This is looking really great. I posted a review on the commit, this
> >>>>> time none of them are style related, it's all pure functionality. The code
> >>>>> looks exactly how I'd imagined writing it myself, so I'm confident we can
> >>>>> get this merged really fast.
> >>>>>
> >>>>> On Mon, May 28, 2012 at 8:52 PM, Jim <jim...@fastmail.co.uk> wrote:
> >>>>>
> >>>>>> The google code web UI does a good job of doing a colour coded diff
> >>>>>> between the added ChainSplitTest and deleted ChainSplitTests.
> >>>>>>
> >>>>>> If you look at:

> >>>>>> http://code.google.com/r/**jimbu**rton618-bitcoinj-**coinbase-tx/**
> >>>>>> source/detail?r=**3b507115ccf1ad**e6423cce21a10251**59bedd5192&**
> >>>>>> name=coinbase-**phase2<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=3b507115ccf1ade6423cce21a1025159bedd5192&name=coinbase-phase2>

[Mike Hearn]

On Fri, Jun 1, 2012 at 5:44 PM, Jim <jim...@fastmail.co.uk> wrote:

Great,
I'll start on the last bit to see if outputs are spendable and coinbase
tx in reorgs over the weekend.

Thanks. You might want to introduce an isMature() method on Transaction.

Can I beg a review of the pubkey-script changes? You need this for coinbase transactions because they are OP_CHECKSIG outputs, which are only supported via these patches. 

[Jim Burton]

I have made a bit of progress on the coinbase tx:

1) The transaction now has a "isMature()" method on indicating whether they are spendable yet.

2) There is a networkParameters.getSpendableCoinbaseDepth() equal to 120 blocks for prodnet, 5 for testnet.

3) The wallet balance takes the tx maturity into account. At the moment an immature tx appears on neither the estimated or available balance. I think it should probably appear in the estimated balance so will change that.

4) Once the coinbase are mature you can create a spend with them. (I still need to write a test that they *really* spend).

There is a branch "coinbase-phase3" with Mike's pubkeyscripts work merged in and the work I have done so far here:

http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/list?name=coinbase-phase3

There is a test BlockChainTest#testCoinbaseTransactionAvailablilty to test it.

There is still work to do (coinbase tx dying in a reorg, more tests and tidying up) so do not bother code reviewing the branch yet - I just wanted to share the progress.

:-)

[Mike Hearn]

Cool, thanks for the update. Sounds like good progress!

[Jim Burton]

In preparation for the coinbase-phase3 patch, I have done a commit against master which simply renames TransactionConfidence.OVERRIDDEN_BY_DOUBLE_SPEND to 

TransactionConfidence.DEAD

It will make the commit that actually does the work smaller and easier to review if I rebase against a master with the rename in.

The changeset is here:

http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=584141ab0321feebdda54e32fd7fbe8689af99e6&name=bitcoinj-master-with-rename

It is the branch : bitcoinj-master-with-rename

          commit : 584141ab0321

For this commit I just wanted to double check a change to bitcoin.proto. I have relabelled the TransactionConfidence.OVERRIDDEN_BY_DOUBLE_SPEND to DEAD but the enum number has not changed. I believe this means that any

previously stored wallet will load properly.

Once that goes in I will rebase against the new master and post up a commit for review.

[Mike Hearn]

Yep that's right. It's an API change but not a format change.

[Mike Hearn]

OK, merged.

[Jim Burton]

I have made a commit for the 'coinbase-phase3' work which includes:

+ checking coinbase tx are spendable in Wallet#completeTx

+ adding a Transaction#isMature so that you can tell when a tx has matured (i.e coinbase depth > minimum required)

+ adjusting the wallet balance so that coinbase tx always appear in the estimated balance but only appear in the available balance when mature.

+ dealing with coinbase death on reorgs

The commit is: e2718f0c28d9

The changeset is here:

http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=e2718f0c28d972c20f8263db89992f2b9a299a44&name=coinbase-phase3-rebase#

This commit finishes all the items in Mike's email describing the work for coinbase support (unless we have missed something of course!)

There are tests as follows:

+ BlockChainTest#coinbaseTransactionAvailibility - checks the maturation of coinbase tx and the value of transaction.isMature()

+ ChainSplitTest#coinbaseDeath - test that coinbase tx get TransactionConfidence.DEAD confidence when moved to a sidechain and get TransactionConfidence.BUILDING when they move back to the best chain.

+ WalletTest#coinbaseTransactionSpend - test that you can actually spend a mature coinbase tx and it gets received to a different wallet ok

+ CoinbaseBlockTest#testReceiveCoinbaseTransaction - test that a production coinbase I bought off a miner gets input to a wallet correctly. (This was produced in fixing a bug in MultiBit but is a useful real world test so I have moved it into bitcoinj)

The tests all run ok.

Cheers

[Mike Hearn]

Thanks Jim!

I posted a review on that changelist.

[Jim Burton]

Hi Mike,
I have gone through today's review comments and created a new coinbase-tx commit code accordingly.
It is here:

Commit: ad2cea535d3e
http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=ad2cea535d3e3d999c34434a327a5de56812b32f&name=coinbase-phase3-rebase#



The only non-obvious change is around the Wallet#reprocessUnincludedTxAfterReorg method.
Changes:
+ The coinbase tx are treated just the same as the other tx.
+ This code tweak has been deleted:

- if (input.isCoinBase()) {
- // Input is not in our wallet so there is "no such input tx", bit of an abuse.
- noSuchTx++;
- continue;
- }

As the coinbase tx are now in the wallet they can be input to other txes in the wallet just like any other - no special casing required.
+ Coinbase can appear in reprocessUnincludedTxAfterReorg so I have not put in a Precondition about them as we discussed via email.

The tests all run cleanly.

[Mike Hearn]

Thanks.

I am still confused about the change to reprocessUnincludedTxAfterReorg.

During re-org any coinbases that spend to us will be moved to dead -> this is fine.

We then process each dead tx and attempt to connect its input -> this will always fail, leading to noSuchTx == numInputs

if (noSuchTx == numInputs) the coinbase will be moved back out of dead and into inactive, which is incorrect.

Am I wrong?

[Jim]

Hi Mike,

Just having a look at your email about dead coinbase tx processing.
You are correct, the code is wrong.
Looking at the CoinSplitTest#coinbaseDeath test, just after the chain
gets reorged to:

// genesis -> b1 -> b2 -> b3
// \-> b4 -> b5 -> b6

The status of the dead tx in block b2 is:
11:02:49 10 Wallet.reprocessUnincludedTxAfterReorg: TX
b5064848d416ffddfa7f67907bac328f5acf2fb7973f110f50803113fa251177,
confidence = DEAD
11:02:49 10 Wallet.reprocessUnincludedTxAfterReorg: ->inactive

It's confidence is TransactionConfidence.DEAD but it is now in the
inactive pool. Oh dear!

By cracking open the scope of the Wallet.dead pool from private to
package level I have added tests that the tx is both
TransactionConfidence.DEAD and in the dead pool so at least the
CoinSplitTest#coinbaseDeath fails now i.e. it captures the bug.

It does not feel right being able to put a tx labeled
TransactionConfidence.DEAD into a pool other than dead so if you can
suggest improvements here it would be appreciated. Reorgs are
complicated enough without us trying to bamboozle ourselves with having
two similar-but-different tx classifications.

I will have a bit more of a look at it today but want to get a MultiBit
release out early next week so it might have to slip to after that.

Jim

> >>> The commit is: *e2718f0c28d9*
> >>> The changeset is here:
> >>> http://code.google.com/r/**jimburton618-bitcoinj-**
> >>> coinbase-tx/source/detail?r=**e2718f0c28d972c20f8263db89992f**
> >>> 2b9a299a44&name=coinbase-**phase3-rebase#<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=e2718f0c28d972c20f8263db89992f2b9a299a44&name=coinbase-phase3-rebase#>

> >>>
> >>> This commit finishes all the items in Mike's email describing the work
> >>> for coinbase support (unless we have missed something of course!)
> >>>
> >>> There are tests as follows:

> >>> + BlockChainTest#**coinbaseTransactionAvailibilit**y - checks the

> >>> maturation of coinbase tx and the value of transaction.isMature()
> >>> + ChainSplitTest#coinbaseDeath - test that coinbase tx get
> >>> TransactionConfidence.DEAD confidence when moved to a sidechain and get
> >>> TransactionConfidence.BUILDING when they move back to the best chain.

> >>> + WalletTest#**coinbaseTransactionSpend - test that you can actually

> >>> spend a mature coinbase tx and it gets received to a different wallet ok

> >>> + CoinbaseBlockTest#**testReceiveCoinbaseTransaction - test that a

> >>> production coinbase I bought off a miner gets input to a wallet correctly.
> >>> (This was produced in fixing a bug in MultiBit but is a useful real world
> >>> test so I have moved it into bitcoinj)
> >>>
> >>> The tests all run ok.
> >>>
> >>> Cheers
> >>>
> >>>
> >>> On Friday, June 8, 2012 4:59:32 PM UTC+1, Mike Hearn wrote:
> >>>>
> >>>> OK, merged.
> >>>>
> >>>> On Thu, Jun 7, 2012 at 6:45 PM, Mike Hearn <he...@google.com> wrote:
> >>>>
> >>>>> Yep that's right. It's an API change but not a format change.
> >>>>> On Jun 7, 2012 5:30 PM, "Jim Burton" <jim...@fastmail.co.uk> wrote:
> >>>>>
> >>>>>> In preparation for the coinbase-phase3 patch, I have done a commit

> >>>>>> against master which simply renames TransactionConfidence.**OVERRIDD*
> >>>>>> *EN_BY_DOUBLE_SPEND to

> >>>>>> TransactionConfidence.DEAD
> >>>>>>
> >>>>>> It will make the commit that actually does the work smaller and
> >>>>>> easier to review if I rebase against a master with the rename in.
> >>>>>> The changeset is here:
> >>>>>>

> >>>>>> http://code.google.com/r/**jimbu**rton618-bitcoinj-**coinbase-tx/**
> >>>>>> source/detail?r=**584141ab0321fe**ebdda54e32fd7fbe**8689af99e6&**
> >>>>>> name=bitcoinj-**master-with-**rename<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=584141ab0321feebdda54e32fd7fbe8689af99e6&name=bitcoinj-master-with-rename>

> >>>>>>
> >>>>>> It is the branch : bitcoinj-master-with-rename
> >>>>>> commit : 584141ab0321
> >>>>>>

> >>>>>> For this commit I just wanted to* double check* a change to
> >>>>>> bitcoin.proto. I have relabelled the TransactionConfidence.**OVERRIDD
> >>>>>> **EN_BY_DOUBLE_SPEND to DEAD but the enum number *has not* changed.

> >>>>>> I believe this means that any
> >>>>>> previously stored wallet will load properly.
> >>>>>>
> >>>>>> Once that goes in I will rebase against the new master and post up a
> >>>>>> commit for review.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On Wednesday, June 6, 2012 10:42:43 AM UTC+1, Mike Hearn wrote:
> >>>>>>>
> >>>>>>> Cool, thanks for the update. Sounds like good progress!
> >>>>>>>
> >>>>>>> On Tue, Jun 5, 2012 at 9:20 PM, Jim Burton <jim...@fastmail.co.uk>wrote:
> >>>>>>>
> >>>>>>>> I have made a bit of progress on the coinbase tx:
> >>>>>>>>
> >>>>>>>> 1) The transaction now has a "isMature()" method on indicating
> >>>>>>>> whether they are spendable yet.

> >>>>>>>> 2) There is a networkParameters.**getSpendable****CoinbaseDepth()

> >>>>>>>> equal to 120 blocks for prodnet, 5 for testnet.
> >>>>>>>> 3) The wallet balance takes the tx maturity into account. At the
> >>>>>>>> moment an immature tx appears on neither the estimated or available
> >>>>>>>> balance. I think it should probably appear in the estimated balance so will
> >>>>>>>> change that.
> >>>>>>>> 4) Once the coinbase are mature you can create a spend with them.
> >>>>>>>> (I still need to write a test that they *really* spend).
> >>>>>>>>
> >>>>>>>> There is a branch "coinbase-phase3" with Mike's pubkeyscripts work
> >>>>>>>> merged in and the work I have done so far here:

> >>>>>>>> http://code.google.com/r/**jimbu****rton618-bitcoinj-**coinbase-tx/
> >>>>>>>> **s**ource/list?name=**coinbase-**phase**3<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/list?name=coinbase-phase3>
> >>>>>>>>
> >>>>>>>> There is a test BlockChainTest#**testCoinbaseTra****nsactionAvailab
> >>>>>>>> **lilty to test it.

[Mike Hearn]

Yes, I quite agree we can make this more straightforward or safer. The complexity and fragility of the wallet code is unsatisfying to me. Unfortunately it's hard to see obvious wins - Bitcoin is just complex.

[Jim Burton]

I have fixed the issue with coinbase transactions for when they go between the best chain and side chain and back again.

There is a new commit with the coinbase-phase3 code for review here:

http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=38c3f88a76f8eeb76564e9718cd68db63572d86d&name=coinbase-phase3-rebase#

Commit: 38c3f88a76f8

Notes:

+ This commit actually lets the coinbase tx into the wallet. There is a change to Wallet#isTransactionRelevant which previously was filtering them out.

+ The main test that exercises the 'coinbase death and resurrection' functionality is ChainSplitTests#coinbaseDeath.

+ I deleted BlockChain#scanTransactions as it was private but not actually being used.

tl;dr;

You can spend your mining dues!

[Mike Hearn]

Great! I added a few more review comments, but the vast majority are just typos or comment clarity requests.

The only remaining point I'm confused about, is the movement of coinbase transactions in and out of the pending pool. I thought that did not happen.

[Mike Hearn]

Hey Jim,

We're nearly there ... any idea when you'll next get a chance to look at my last comments?

[Jim]

Hi Mike,

I was going to have a look today.

:-)

Jim

On Wed, Jul 4, 2012, at 01:39 PM, Mike Hearn wrote:
> Hey Jim,
>
> We're nearly there ... any idea when you'll next get a chance to look at
> my
> last comments?
>
>
> On Wed, Jun 27, 2012 at 12:11 PM, Mike Hearn <he...@google.com> wrote:
>
> > Great! I added a few more review comments, but the vast majority are just
> > typos or comment clarity requests.
> >
> > The only remaining point I'm confused about, is the movement of coinbase
> > transactions in and out of the pending pool. I thought that did not happen.
> >
> >
> > On Mon, Jun 25, 2012 at 1:46 PM, Jim Burton <jim...@fastmail.co.uk> wrote:
> >
> >> I have fixed the issue with coinbase transactions for when they go
> >> between the best chain and side chain and back again.
> >> There is a new commit with the coinbase-phase3 code for review here:
> >>
> >>
> >> http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=38c3f88a76f8eeb76564e9718cd68db63572d86d&name=coinbase-phase3-rebase#
> >>

> >> Commit: *38c3f88a76f8*

> >>
> >> Notes:
> >> + This commit actually lets the coinbase tx into the wallet. There is a
> >> change to Wallet#isTransactionRelevant which previously was filtering them
> >> out.
> >> + The main test that exercises the 'coinbase death and resurrection'
> >> functionality is ChainSplitTests#coinbaseDeath.
> >> + I deleted BlockChain#scanTransactions as it was private but not
> >> actually being used.
> >>
> >> tl;dr;
> >> You can spend your mining dues!
> >>
> >>
> >> On Monday, June 18, 2012 7:27:40 PM UTC+1, Mike Hearn wrote:
> >>>
> >>> Yes, I quite agree we can make this more straightforward or safer. The
> >>> complexity and fragility of the wallet code is unsatisfying to me.
> >>> Unfortunately it's hard to see obvious wins - Bitcoin is just complex.
> >>>
> >>> On Sat, Jun 16, 2012 at 12:24 PM, Jim <jim...@fastmail.co.uk> wrote:
> >>>
> >>>> Hi Mike,
> >>>>
> >>>> Just having a look at your email about dead coinbase tx processing.
> >>>> You are correct, the code is wrong.
> >>>> Looking at the CoinSplitTest#coinbaseDeath test, just after the chain
> >>>> gets reorged to:
> >>>>
> >>>> // genesis -> b1 -> b2 -> b3
> >>>> // \-> b4 -> b5 -> b6
> >>>>
> >>>> The status of the dead tx in block b2 is:

> >>>> 11:02:49 10 Wallet.**reprocessUnincludedTxAfterReor**g: TX
> >>>> b5064848d416ffddfa7f67907bac32**8f5acf2fb7973f110f50803113fa25**1177,
> >>>> confidence = DEAD
> >>>> 11:02:49 10 Wallet.**reprocessUnincludedTxAfterReor**g: ->inactive

> >>>>
> >>>> It's confidence is TransactionConfidence.DEAD but it is now in the
> >>>> inactive pool. Oh dear!
> >>>>
> >>>> By cracking open the scope of the Wallet.dead pool from private to
> >>>> package level I have added tests that the tx is both
> >>>> TransactionConfidence.DEAD and in the dead pool so at least the
> >>>> CoinSplitTest#coinbaseDeath fails now i.e. it captures the bug.
> >>>>
> >>>> It does not feel right being able to put a tx labeled
> >>>> TransactionConfidence.DEAD into a pool other than dead so if you can
> >>>> suggest improvements here it would be appreciated. Reorgs are
> >>>> complicated enough without us trying to bamboozle ourselves with having
> >>>> two similar-but-different tx classifications.
> >>>>
> >>>> I will have a bit more of a look at it today but want to get a MultiBit
> >>>> release out early next week so it might have to slip to after that.
> >>>>
> >>>> Jim
> >>>>
> >>>>
> >>>>
> >>>> On Fri, Jun 15, 2012, at 11:17 AM, Mike Hearn wrote:
> >>>> > Thanks.
> >>>> >
> >>>> > I am still confused about the change to reprocessUnincludedTxAfterReor

> >>>> **g.

> >>>> >
> >>>> > During re-org any coinbases that spend to us will be moved to dead ->
> >>>> > this
> >>>> > is fine.
> >>>> >
> >>>> > We then process each dead tx and attempt to connect its input -> this
> >>>> > will
> >>>> > always fail, leading to noSuchTx == numInputs
> >>>> >
> >>>> > if (noSuchTx == numInputs) the coinbase will be moved back out of dead
> >>>> > and
> >>>> > into inactive, which is incorrect.
> >>>> >
> >>>> > Am I wrong?
> >>>> >
> >>>> > On Thu, Jun 14, 2012 at 8:32 PM, Jim Burton <jim...@fastmail.co.uk>
> >>>> > wrote:
> >>>> >
> >>>> > > Hi Mike,
> >>>> > > I have gone through today's review comments and created a new
> >>>> coinbase-tx
> >>>> > > commit code accordingly.
> >>>> > > It is here:
> >>>> > >
> >>>> > > Commit: ad2cea535d3e
> >>>> > >

> >>>> > > http://code.google.com/r/**jimburton618-bitcoinj-**
> >>>> coinbase-tx/source/detail?r=**ad2cea535d3e3d999c34434a327a5d**
> >>>> e56812b32f&name=coinbase-**phase3-rebase#<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=ad2cea535d3e3d999c34434a327a5de56812b32f&name=coinbase-phase3-rebase#>

> >>>> > >
> >>>> > >
> >>>> > >
> >>>> > > The only non-obvious change is around the

> >>>> > > Wallet#**reprocessUnincludedTxAfterReor**g method.

> >>>> > > Changes:
> >>>> > > + The coinbase tx are treated just the same as the other tx.
> >>>> > > + This code tweak has been deleted:
> >>>> > >
> >>>> > > - if (input.isCoinBase()) {
> >>>> > > - // Input is not in our wallet so there is "no such input tx", bit
> >>>> of an
> >>>> > > abuse.
> >>>> > > - noSuchTx++;
> >>>> > > - continue;
> >>>> > > - }
> >>>> > >
> >>>> > > As the coinbase tx are now in the wallet they can be input to other
> >>>> txes
> >>>> > > in the wallet just like any other - no special casing required.

> >>>> > > + Coinbase can appear in reprocessUnincludedTxAfterReor**g so I

> >>>> have not put
> >>>> > > in a Precondition about them as we discussed via email.
> >>>> > >
> >>>> > > The tests all run cleanly.
> >>>> > >
> >>>> > >
> >>>> > >
> >>>> > >
> >>>> > >
> >>>> > > On Wednesday, June 13, 2012 5:11:06 PM UTC+1, Mike Hearn wrote:
> >>>> > >>
> >>>> > >> Thanks Jim!
> >>>> > >>
> >>>> > >> I posted a review on that changelist.
> >>>> > >>
> >>>> > >> On Sat, Jun 9, 2012 at 1:31 PM, Jim Burton <jim...@fastmail.co.uk>
> >>>> wrote:
> >>>> > >>
> >>>> > >>> I have made a commit for the 'coinbase-phase3' work which
> >>>> includes:
> >>>> > >>> + checking coinbase tx are spendable in Wallet#completeTx
> >>>> > >>> + adding a Transaction#isMature so that you can tell when a tx
> >>>> has
> >>>> > >>> matured (i.e coinbase depth > minimum required)
> >>>> > >>> + adjusting the wallet balance so that coinbase tx always appear
> >>>> in the
> >>>> > >>> estimated balance but only appear in the available balance when
> >>>> mature.
> >>>> > >>> + dealing with coinbase death on reorgs
> >>>> > >>>
> >>>> > >>> The commit is: *e2718f0c28d9*
> >>>> > >>> The changeset is here:

> >>>> > >>> http://code.google.com/r/****jimburton618-bitcoinj-**<http://code.google.com/r/**jimburton618-bitcoinj-**>
> >>>> > >>> coinbase-tx/source/detail?r=****e2718f0c28d972c20f8263db89992f**
> >>>> **
> >>>> > >>> 2b9a299a44&name=coinbase-****phase3-rebase#<http://code.**
> >>>> google.com/r/jimburton618-**bitcoinj-coinbase-tx/source/**detail?r=**
> >>>> e2718f0c28d972c20f8263db89992f**2b9a299a44&name=coinbase-**

> >>>> phase3-rebase#<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=e2718f0c28d972c20f8263db89992f2b9a299a44&name=coinbase-phase3-rebase#>
> >>>> >
> >>>> > >>>
> >>>> > >>> This commit finishes all the items in Mike's email describing the
> >>>> work
> >>>> > >>> for coinbase support (unless we have missed something of course!)
> >>>> > >>>
> >>>> > >>> There are tests as follows:

> >>>> > >>> + BlockChainTest#****coinbaseTransactionAvailibilit****y -

> >>>> checks the
> >>>> > >>> maturation of coinbase tx and the value of transaction.isMature()
> >>>> > >>> + ChainSplitTest#coinbaseDeath - test that coinbase tx get
> >>>> > >>> TransactionConfidence.DEAD confidence when moved to a sidechain
> >>>> and get
> >>>> > >>> TransactionConfidence.BUILDING when they move back to the best
> >>>> chain.

> >>>> > >>> + WalletTest#****coinbaseTransactionSpend - test that you can

> >>>> actually
> >>>> > >>> spend a mature coinbase tx and it gets received to a different
> >>>> wallet ok

> >>>> > >>> + CoinbaseBlockTest#****testReceiveCoinbaseTransaction - test

> >>>> that a
> >>>> > >>> production coinbase I bought off a miner gets input to a wallet
> >>>> correctly.
> >>>> > >>> (This was produced in fixing a bug in MultiBit but is a useful
> >>>> real world
> >>>> > >>> test so I have moved it into bitcoinj)
> >>>> > >>>
> >>>> > >>> The tests all run ok.
> >>>> > >>>
> >>>> > >>> Cheers
> >>>> > >>>
> >>>> > >>>
> >>>> > >>> On Friday, June 8, 2012 4:59:32 PM UTC+1, Mike Hearn wrote:
> >>>> > >>>>
> >>>> > >>>> OK, merged.
> >>>> > >>>>
> >>>> > >>>> On Thu, Jun 7, 2012 at 6:45 PM, Mike Hearn <he...@google.com>
> >>>> wrote:
> >>>> > >>>>
> >>>> > >>>>> Yep that's right. It's an API change but not a format change.
> >>>> > >>>>> On Jun 7, 2012 5:30 PM, "Jim Burton" <jim...@fastmail.co.uk>
> >>>> wrote:
> >>>> > >>>>>
> >>>> > >>>>>> In preparation for the coinbase-phase3 patch, I have done a
> >>>> commit

> >>>> > >>>>>> against master which simply renames TransactionConfidence.****
> >>>> OVERRIDD*

> >>>> > >>>>>> *EN_BY_DOUBLE_SPEND to
> >>>> > >>>>>> TransactionConfidence.DEAD
> >>>> > >>>>>>
> >>>> > >>>>>> It will make the commit that actually does the work smaller
> >>>> and
> >>>> > >>>>>> easier to review if I rebase against a master with the rename
> >>>> in.
> >>>> > >>>>>> The changeset is here:
> >>>> > >>>>>>

> >>>> > >>>>>> http://code.google.com/r/****jimbu**rton618-bitcoinj-****
> >>>> coinbase-tx/**<http://code.google.com/r/**jimbu**rton618-bitcoinj-**coinbase-tx/**>
> >>>> > >>>>>> source/detail?r=****584141ab0321fe****
> >>>> ebdda54e32fd7fbe**8689af99e6&****
> >>>> > >>>>>> name=bitcoinj-**master-with-****rename<http://code.google.com/
> >>>> **r/jimburton618-bitcoinj-**coinbase-tx/source/detail?r=**
> >>>> 584141ab0321feebdda54e32fd7fbe**8689af99e6&name=bitcoinj-**
> >>>> master-with-rename<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=584141ab0321feebdda54e32fd7fbe8689af99e6&name=bitcoinj-master-with-rename>

> >>>> >
> >>>> > >>>>>>
> >>>> > >>>>>> It is the branch : bitcoinj-master-with-rename
> >>>> > >>>>>> commit : 584141ab0321
> >>>> > >>>>>>
> >>>> > >>>>>> For this commit I just wanted to* double check* a change to

> >>>> > >>>>>> bitcoin.proto. I have relabelled the TransactionConfidence.***
> >>>> *OVERRIDD

> >>>> > >>>>>> **EN_BY_DOUBLE_SPEND to DEAD but the enum number *has not*
> >>>> changed.
> >>>> > >>>>>> I believe this means that any
> >>>> > >>>>>> previously stored wallet will load properly.
> >>>> > >>>>>>
> >>>> > >>>>>> Once that goes in I will rebase against the new master and
> >>>> post up a
> >>>> > >>>>>> commit for review.
> >>>> > >>>>>>
> >>>> > >>>>>>
> >>>> > >>>>>>
> >>>> > >>>>>> On Wednesday, June 6, 2012 10:42:43 AM UTC+1, Mike Hearn wrote:
> >>>> > >>>>>>>
> >>>> > >>>>>>> Cool, thanks for the update. Sounds like good progress!
> >>>> > >>>>>>>
> >>>> > >>>>>>> On Tue, Jun 5, 2012 at 9:20 PM, Jim Burton <
> >>>> jim...@fastmail.co.uk>wrote:
> >>>> > >>>>>>>
> >>>> > >>>>>>>> I have made a bit of progress on the coinbase tx:
> >>>> > >>>>>>>>
> >>>> > >>>>>>>> 1) The transaction now has a "isMature()" method on
> >>>> indicating
> >>>> > >>>>>>>> whether they are spendable yet.

> >>>> > >>>>>>>> 2) There is a networkParameters.****
> >>>> getSpendable****CoinbaseDepth(**)

> >>>> > >>>>>>>> equal to 120 blocks for prodnet, 5 for testnet.
> >>>> > >>>>>>>> 3) The wallet balance takes the tx maturity into account. At
> >>>> the
> >>>> > >>>>>>>> moment an immature tx appears on neither the estimated or
> >>>> available
> >>>> > >>>>>>>> balance. I think it should probably appear in the estimated
> >>>> balance so will
> >>>> > >>>>>>>> change that.
> >>>> > >>>>>>>> 4) Once the coinbase are mature you can create a spend with
> >>>> them.
> >>>> > >>>>>>>> (I still need to write a test that they *really* spend).
> >>>> > >>>>>>>>
> >>>> > >>>>>>>> There is a branch "coinbase-phase3" with Mike's
> >>>> pubkeyscripts work
> >>>> > >>>>>>>> merged in and the work I have done so far here:

> >>>> > >>>>>>>> http://code.google.com/r/****jimbu****rton618-bitcoinj-****
> >>>> coinbase-tx/<http://code.google.com/r/**jimbu****rton618-bitcoinj-**coinbase-tx/>
> >>>> > >>>>>>>> **s**ource/list?name=****coinbase-**phase**3<http://**
> >>>> code.google.com/r/**jimburton618-bitcoinj-**
> >>>> coinbase-tx/source/list?name=**coinbase-phase3<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/list?name=coinbase-phase3>
> >>>> >
> >>>> > >>>>>>>>
> >>>> > >>>>>>>> There is a test BlockChainTest#****testCoinbaseTra******
> >>>> nsactionAvailab

[Jim]

Hi Mike,

Just been looking at the 'dead coinbase resurrection' code in
Wallet#reorganise when it moves dead->pending.
I think it just needs the following:

boolean isDeadCoinbase = t.isCoinBase() &&
dead.containsKey(t.getHash());
if (isDeadCoinbase) {
// There is a dead coinbase tx being received on the best chain.
// Take it out of the dead pool.
// The receive method will then treat it as a new transaction and
put it in spent or unspent as appropriate.
log.info(" coinbaseTX {} <-dead", t.getHashAsString() + ",
confidence = " + t.getConfidence().getConfidenceType().name());
dead.remove(t.getHash());
}
receive(t, b, BlockChain.NewBlockType.BEST_CHAIN, true);


In receive it then gets passed to processTxFromBestChain(tx) where it
gets processed and tx confidence listeners get fired as necessary in
updateForSpend.

The tests all pass with this but, more importantly, it is more logical
as it does not make sense for coinbase tx to ever be pending (They
'existentially' have to be in a block)

Jim.

P.S. Assuming you are happy with this, I will do the comment tidy ups
etc from your review this afternoon/ this evening and prep a commit for
you.

On Wed, Jun 27, 2012, at 12:11 PM, Mike Hearn wrote:
> Great! I added a few more review comments, but the vast majority are just
> typos or comment clarity requests.
>
> The only remaining point I'm confused about, is the movement of coinbase
> transactions in and out of the pending pool. I thought that did not
> happen.
>
>
> On Mon, Jun 25, 2012 at 1:46 PM, Jim Burton <jim...@fastmail.co.uk>
> wrote:
>
> > I have fixed the issue with coinbase transactions for when they go between
> > the best chain and side chain and back again.
> > There is a new commit with the coinbase-phase3 code for review here:
> >
> >
> > http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=38c3f88a76f8eeb76564e9718cd68db63572d86d&name=coinbase-phase3-rebase#
> >

> > Commit: *38c3f88a76f8*

> >
> > Notes:
> > + This commit actually lets the coinbase tx into the wallet. There is a
> > change to Wallet#isTransactionRelevant which previously was filtering them
> > out.
> > + The main test that exercises the 'coinbase death and resurrection'
> > functionality is ChainSplitTests#coinbaseDeath.
> > + I deleted BlockChain#scanTransactions as it was private but not actually
> > being used.
> >
> > tl;dr;
> > You can spend your mining dues!
> >
> >
> > On Monday, June 18, 2012 7:27:40 PM UTC+1, Mike Hearn wrote:
> >>
> >> Yes, I quite agree we can make this more straightforward or safer. The
> >> complexity and fragility of the wallet code is unsatisfying to me.
> >> Unfortunately it's hard to see obvious wins - Bitcoin is just complex.
> >>
> >> On Sat, Jun 16, 2012 at 12:24 PM, Jim <jim...@fastmail.co.uk> wrote:
> >>
> >>> Hi Mike,
> >>>
> >>> Just having a look at your email about dead coinbase tx processing.
> >>> You are correct, the code is wrong.
> >>> Looking at the CoinSplitTest#coinbaseDeath test, just after the chain
> >>> gets reorged to:
> >>>
> >>> // genesis -> b1 -> b2 -> b3
> >>> // \-> b4 -> b5 -> b6
> >>>
> >>> The status of the dead tx in block b2 is:

> >>> 11:02:49 10 Wallet.**reprocessUnincludedTxAfterReor**g: TX
> >>> b5064848d416ffddfa7f67907bac32**8f5acf2fb7973f110f50803113fa25**1177,
> >>> confidence = DEAD
> >>> 11:02:49 10 Wallet.**reprocessUnincludedTxAfterReor**g: ->inactive

> >>>
> >>> It's confidence is TransactionConfidence.DEAD but it is now in the
> >>> inactive pool. Oh dear!
> >>>
> >>> By cracking open the scope of the Wallet.dead pool from private to
> >>> package level I have added tests that the tx is both
> >>> TransactionConfidence.DEAD and in the dead pool so at least the
> >>> CoinSplitTest#coinbaseDeath fails now i.e. it captures the bug.
> >>>
> >>> It does not feel right being able to put a tx labeled
> >>> TransactionConfidence.DEAD into a pool other than dead so if you can
> >>> suggest improvements here it would be appreciated. Reorgs are
> >>> complicated enough without us trying to bamboozle ourselves with having
> >>> two similar-but-different tx classifications.
> >>>
> >>> I will have a bit more of a look at it today but want to get a MultiBit
> >>> release out early next week so it might have to slip to after that.
> >>>
> >>> Jim
> >>>
> >>>
> >>>
> >>> On Fri, Jun 15, 2012, at 11:17 AM, Mike Hearn wrote:
> >>> > Thanks.
> >>> >
> >>> > I am still confused about the change to reprocessUnincludedTxAfterReor

> >>> **g.

> >>> >
> >>> > During re-org any coinbases that spend to us will be moved to dead ->
> >>> > this
> >>> > is fine.
> >>> >
> >>> > We then process each dead tx and attempt to connect its input -> this
> >>> > will
> >>> > always fail, leading to noSuchTx == numInputs
> >>> >
> >>> > if (noSuchTx == numInputs) the coinbase will be moved back out of dead
> >>> > and
> >>> > into inactive, which is incorrect.
> >>> >
> >>> > Am I wrong?
> >>> >
> >>> > On Thu, Jun 14, 2012 at 8:32 PM, Jim Burton <jim...@fastmail.co.uk>
> >>> > wrote:
> >>> >
> >>> > > Hi Mike,
> >>> > > I have gone through today's review comments and created a new
> >>> coinbase-tx
> >>> > > commit code accordingly.
> >>> > > It is here:
> >>> > >
> >>> > > Commit: ad2cea535d3e
> >>> > >

> >>> > > http://code.google.com/r/**jimburton618-bitcoinj-**
> >>> coinbase-tx/source/detail?r=**ad2cea535d3e3d999c34434a327a5d**
> >>> e56812b32f&name=coinbase-**phase3-rebase#<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=ad2cea535d3e3d999c34434a327a5de56812b32f&name=coinbase-phase3-rebase#>

> >>> > >
> >>> > >
> >>> > >
> >>> > > The only non-obvious change is around the

> >>> > > Wallet#**reprocessUnincludedTxAfterReor**g method.

> >>> > > Changes:
> >>> > > + The coinbase tx are treated just the same as the other tx.
> >>> > > + This code tweak has been deleted:
> >>> > >
> >>> > > - if (input.isCoinBase()) {
> >>> > > - // Input is not in our wallet so there is "no such input tx", bit
> >>> of an
> >>> > > abuse.
> >>> > > - noSuchTx++;
> >>> > > - continue;
> >>> > > - }
> >>> > >
> >>> > > As the coinbase tx are now in the wallet they can be input to other
> >>> txes
> >>> > > in the wallet just like any other - no special casing required.

> >>> > > + Coinbase can appear in reprocessUnincludedTxAfterReor**g so I

> >>> have not put
> >>> > > in a Precondition about them as we discussed via email.
> >>> > >
> >>> > > The tests all run cleanly.
> >>> > >
> >>> > >
> >>> > >
> >>> > >
> >>> > >
> >>> > > On Wednesday, June 13, 2012 5:11:06 PM UTC+1, Mike Hearn wrote:
> >>> > >>
> >>> > >> Thanks Jim!
> >>> > >>
> >>> > >> I posted a review on that changelist.
> >>> > >>
> >>> > >> On Sat, Jun 9, 2012 at 1:31 PM, Jim Burton <jim...@fastmail.co.uk>
> >>> wrote:
> >>> > >>
> >>> > >>> I have made a commit for the 'coinbase-phase3' work which includes:
> >>> > >>> + checking coinbase tx are spendable in Wallet#completeTx
> >>> > >>> + adding a Transaction#isMature so that you can tell when a tx has
> >>> > >>> matured (i.e coinbase depth > minimum required)
> >>> > >>> + adjusting the wallet balance so that coinbase tx always appear
> >>> in the
> >>> > >>> estimated balance but only appear in the available balance when
> >>> mature.
> >>> > >>> + dealing with coinbase death on reorgs
> >>> > >>>
> >>> > >>> The commit is: *e2718f0c28d9*
> >>> > >>> The changeset is here:

> >>> > >>> http://code.google.com/r/****jimburton618-bitcoinj-**<http://code.google.com/r/**jimburton618-bitcoinj-**>
> >>> > >>> coinbase-tx/source/detail?r=****e2718f0c28d972c20f8263db89992f****

> >>> > >>> 2b9a299a44&name=coinbase-****phase3-rebase#<http://code.**
> >>> google.com/r/jimburton618-**bitcoinj-coinbase-tx/source/**detail?r=**
> >>> e2718f0c28d972c20f8263db89992f**2b9a299a44&name=coinbase-**

> >>> phase3-rebase#<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=e2718f0c28d972c20f8263db89992f2b9a299a44&name=coinbase-phase3-rebase#>
> >>> >
> >>> > >>>
> >>> > >>> This commit finishes all the items in Mike's email describing the
> >>> work
> >>> > >>> for coinbase support (unless we have missed something of course!)
> >>> > >>>
> >>> > >>> There are tests as follows:

> >>> > >>> + BlockChainTest#****coinbaseTransactionAvailibilit****y - checks

> >>> the
> >>> > >>> maturation of coinbase tx and the value of transaction.isMature()
> >>> > >>> + ChainSplitTest#coinbaseDeath - test that coinbase tx get
> >>> > >>> TransactionConfidence.DEAD confidence when moved to a sidechain
> >>> and get
> >>> > >>> TransactionConfidence.BUILDING when they move back to the best
> >>> chain.

> >>> > >>> + WalletTest#****coinbaseTransactionSpend - test that you can

> >>> actually
> >>> > >>> spend a mature coinbase tx and it gets received to a different
> >>> wallet ok

> >>> > >>> + CoinbaseBlockTest#****testReceiveCoinbaseTransaction - test

> >>> that a
> >>> > >>> production coinbase I bought off a miner gets input to a wallet
> >>> correctly.
> >>> > >>> (This was produced in fixing a bug in MultiBit but is a useful
> >>> real world
> >>> > >>> test so I have moved it into bitcoinj)
> >>> > >>>
> >>> > >>> The tests all run ok.
> >>> > >>>
> >>> > >>> Cheers
> >>> > >>>
> >>> > >>>
> >>> > >>> On Friday, June 8, 2012 4:59:32 PM UTC+1, Mike Hearn wrote:
> >>> > >>>>
> >>> > >>>> OK, merged.
> >>> > >>>>
> >>> > >>>> On Thu, Jun 7, 2012 at 6:45 PM, Mike Hearn <he...@google.com>
> >>> wrote:
> >>> > >>>>
> >>> > >>>>> Yep that's right. It's an API change but not a format change.
> >>> > >>>>> On Jun 7, 2012 5:30 PM, "Jim Burton" <jim...@fastmail.co.uk>
> >>> wrote:
> >>> > >>>>>
> >>> > >>>>>> In preparation for the coinbase-phase3 patch, I have done a
> >>> commit

> >>> > >>>>>> against master which simply renames TransactionConfidence.****
> >>> OVERRIDD*

> >>> > >>>>>> *EN_BY_DOUBLE_SPEND to
> >>> > >>>>>> TransactionConfidence.DEAD
> >>> > >>>>>>
> >>> > >>>>>> It will make the commit that actually does the work smaller and
> >>> > >>>>>> easier to review if I rebase against a master with the rename
> >>> in.
> >>> > >>>>>> The changeset is here:
> >>> > >>>>>>

> >>> > >>>>>> http://code.google.com/r/****jimbu**rton618-bitcoinj-****
> >>> coinbase-tx/**<http://code.google.com/r/**jimbu**rton618-bitcoinj-**coinbase-tx/**>
> >>> > >>>>>> source/detail?r=****584141ab0321fe****
> >>> ebdda54e32fd7fbe**8689af99e6&****
> >>> > >>>>>> name=bitcoinj-**master-with-****rename<http://code.google.com/*
> >>> *r/jimburton618-bitcoinj-**coinbase-tx/source/detail?r=**
> >>> 584141ab0321feebdda54e32fd7fbe**8689af99e6&name=bitcoinj-**

> >>> master-with-rename<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=584141ab0321feebdda54e32fd7fbe8689af99e6&name=bitcoinj-master-with-rename>

> >>> >
> >>> > >>>>>>
> >>> > >>>>>> It is the branch : bitcoinj-master-with-rename
> >>> > >>>>>> commit : 584141ab0321
> >>> > >>>>>>
> >>> > >>>>>> For this commit I just wanted to* double check* a change to

> >>> > >>>>>> bitcoin.proto. I have relabelled the TransactionConfidence.****
> >>> OVERRIDD

> >>> > >>>>>> **EN_BY_DOUBLE_SPEND to DEAD but the enum number *has not*
> >>> changed.
> >>> > >>>>>> I believe this means that any
> >>> > >>>>>> previously stored wallet will load properly.
> >>> > >>>>>>
> >>> > >>>>>> Once that goes in I will rebase against the new master and post
> >>> up a
> >>> > >>>>>> commit for review.
> >>> > >>>>>>
> >>> > >>>>>>
> >>> > >>>>>>
> >>> > >>>>>> On Wednesday, June 6, 2012 10:42:43 AM UTC+1, Mike Hearn wrote:
> >>> > >>>>>>>
> >>> > >>>>>>> Cool, thanks for the update. Sounds like good progress!
> >>> > >>>>>>>
> >>> > >>>>>>> On Tue, Jun 5, 2012 at 9:20 PM, Jim Burton <
> >>> jim...@fastmail.co.uk>wrote:
> >>> > >>>>>>>
> >>> > >>>>>>>> I have made a bit of progress on the coinbase tx:
> >>> > >>>>>>>>
> >>> > >>>>>>>> 1) The transaction now has a "isMature()" method on indicating
> >>> > >>>>>>>> whether they are spendable yet.

> >>> > >>>>>>>> 2) There is a networkParameters.****
> >>> getSpendable****CoinbaseDepth(**)

> >>> > >>>>>>>> equal to 120 blocks for prodnet, 5 for testnet.
> >>> > >>>>>>>> 3) The wallet balance takes the tx maturity into account. At
> >>> the
> >>> > >>>>>>>> moment an immature tx appears on neither the estimated or
> >>> available
> >>> > >>>>>>>> balance. I think it should probably appear in the estimated
> >>> balance so will
> >>> > >>>>>>>> change that.
> >>> > >>>>>>>> 4) Once the coinbase are mature you can create a spend with
> >>> them.
> >>> > >>>>>>>> (I still need to write a test that they *really* spend).
> >>> > >>>>>>>>
> >>> > >>>>>>>> There is a branch "coinbase-phase3" with Mike's pubkeyscripts
> >>> work
> >>> > >>>>>>>> merged in and the work I have done so far here:

> >>> > >>>>>>>> http://code.google.com/r/****jimbu****rton618-bitcoinj-****
> >>> coinbase-tx/<http://code.google.com/r/**jimbu****rton618-bitcoinj-**coinbase-tx/>
> >>> > >>>>>>>> **s**ource/list?name=****coinbase-**phase**3<http://**
> >>> code.google.com/r/**jimburton618-bitcoinj-**

> >>> coinbase-tx/source/list?name=**coinbase-phase3<http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/list?name=coinbase-phase3>
> >>> >
> >>> > >>>>>>>>
> >>> > >>>>>>>> There is a test BlockChainTest#****testCoinbaseTra******
> >>> nsactionAvailab

[Mike Hearn]

Hi Mike,

Just been looking at the 'dead coinbase resurrection' code in
Wallet#reorganise when it moves dead->pending.
I think it just needs the following:

boolean isDeadCoinbase = t.isCoinBase() &&
dead.containsKey(t.getHash());
if (isDeadCoinbase) {
    // There is a dead coinbase tx being received on the best chain.
    // Take it out of the dead pool.
    // The receive method will then treat it as a new transaction and
    put it in spent or unspent as appropriate.
    log.info(" coinbaseTX {} <-dead", t.getHashAsString() + ",
    confidence = " + t.getConfidence().getConfidenceType().name());
    dead.remove(t.getHash());
}
receive(t, b, BlockChain.NewBlockType.BEST_CHAIN, true);


In receive it then gets passed to processTxFromBestChain(tx) where it
gets processed and tx confidence listeners get fired as necessary in
updateForSpend.

It seems weird to take it out in one place and put it back in a pool in another place. Why not just check for this in processTxFromBestChain?

[Jim]

Yes, that is cleaner. Have moved the code to processTxFromBestChain.

[Mike Hearn]

OK, great. 

I just merged Mirons netty code, so you'll need to rebase. I don't think there'll be many conflicts, if any, as you are touching different parts of the code.

[Mike Hearn]

Actually, can you hold off the rebase for a bit? I just realized that a bugfix I made for an observed problem in WalletTool broke some unit tests. Ugh. Seems Mirons change wasn't fully tested after the last round of review.

[Jim]

I feel another release of bitcoinj approaching ! 

:-)

jim...@fastmail.co.uk

[Jim]

I will hold off the rebase until you give the thumbs up. 

jim...@fastmail.co.uk

[Mike Hearn]

OK, that was an easier fix than I anticipated. I force pushed over upstream master which I'd normally never do, but as the window of time for issues was small it's no biggie in this case. Go ahead and rebase.

[Jim Burton]

coinbase tx patch - I have done a rebase and updated the code with Mike's comments.

The change list is here:

http://code.google.com/r/jimburton618-bitcoinj-coinbase-tx/source/detail?r=4d1e6259a7d845c40374f265e89db50d223fb962&name=coinbase-phase3-rebase#

The commit is: 4d1e6259a7d8

The tests all run cleanly.

[Mike Hearn]

Merged, congrats!

There were a couple of minor issues that I fixed with follow up commits. Otherwise, I synced a wallet with some coinbase transactions in it, apparently without issue (beyond the from address thing Andreas already reported).