bitcoinj icon indicating copy to clipboard operation
bitcoinj copied to clipboard

TransactionOutPoint: store connectedOutput when storing fromTx

Open msgilligan opened this issue 2 years ago • 4 comments

This requires related changes:

  • Clear connectedOutput when clearing fromTx
  • Change comparison in TransactionInputTest to reflect new behavior

Depends on PR #2737

also note that this breaks a test:

org.bitcoinj.core.MemoryFullPrunedBlockChainTest > testGeneratedChain FAILED
    java.lang.IndexOutOfBoundsException: Index 42 out of bounds for length 1
        at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:64)
        at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:70)
        at java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:266)
        at java.base/java.util.Objects.checkIndex(Objects.java:359)
        at java.base/java.util.ArrayList.get(ArrayList.java:427)
        at org.bitcoinj.core.Transaction.getOutput(Transaction.java:1632)
        at org.bitcoinj.core.TransactionOutPoint.<init>(TransactionOutPoint.java:83)
        at org.bitcoinj.core.FullBlockTestGenerator.getBlocksToTest(FullBlockTestGenerator.java:1135)

The test fails because it is actually creating an invalid OutPoint (index 42 for a transaction with only 1 output) and because we are storing the tx and output validation is triggered that otherwise wouldn't happen.

msgilligan avatar Apr 04 '23 03:04 msgilligan

I added an additional commit that detects the invalid index a few lines earlier.

msgilligan avatar Apr 04 '23 03:04 msgilligan

I had to investigate into this territory in my quest to eliminate getParentTransaction() from input and output.

#2997 makes connectedOutput and fromTx immutable and introduces methods for the cases where those need mutation. It doesn't fail any tests.

What do you think, should I rebase mine on yours?

schildbach avatar Apr 08 '23 12:04 schildbach

Thinking a bit into the future, I'm not sure if storing connectedOutput makes sense any more. If all goes well, a transaction output will only be the touple value and script. No index, no parent transaction. For everything that involves how/where a TransactionOutput is being used, you'd need to go through Ttransaction anyway.

On the other hand, and I think this may be responsible for the test failures, for the business logic currently it seems to be a difference if fromTx is set or if connectedOutput is set. The latter case seems to be used for UTXO-based wallets exclusively. We need to work that out I guess.

schildbach avatar Apr 08 '23 12:04 schildbach