From 388ca037ef5e6211d5f8ba4ec5b58f4514c7b4fa Mon Sep 17 00:00:00 2001 From: Sean Gilligan Date: Sun, 9 Aug 2020 22:21:02 -0700 Subject: [PATCH] PrefixedChecksummedBytes hierarchy: Add Comparable interface to Address, remove from PrefixedChecksummedBytes. Requires address subclasses to implement compareTo() and provide the compareAddressPartial() method for comparing the first two fields. This changes the natural ordering of addresses, and removes the natural ordering entirely for other PrefixedChecksummedBytes subclasses. This also fixes a compareTo() collision regarding P2SH and non-P2SH addresses with the same bytes[]. --- .../main/java/org/bitcoinj/core/Address.java | 42 ++++++++++++++++++- .../java/org/bitcoinj/core/LegacyAddress.java | 17 ++++++++ .../core/PrefixedChecksummedBytes.java | 12 +----- .../java/org/bitcoinj/core/SegwitAddress.java | 16 +++++++ .../org/bitcoinj/core/LegacyAddressTest.java | 10 +++++ .../core/PrefixedChecksummedBytesTest.java | 3 +- 6 files changed, 87 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/org/bitcoinj/core/Address.java b/core/src/main/java/org/bitcoinj/core/Address.java index 9437a0419..db6342fe2 100644 --- a/core/src/main/java/org/bitcoinj/core/Address.java +++ b/core/src/main/java/org/bitcoinj/core/Address.java @@ -31,7 +31,7 @@ import org.bitcoinj.script.Script.ScriptType; * form. *

*/ -public abstract class Address extends PrefixedChecksummedBytes { +public abstract class Address extends PrefixedChecksummedBytes implements Comparable
{ public Address(NetworkParameters params, byte[] bytes) { super(params, bytes); } @@ -101,4 +101,44 @@ public abstract class Address extends PrefixedChecksummedBytes { * @return type of output script */ public abstract ScriptType getOutputScriptType(); + + /** + * Comparison field order for addresses is: + *
    + *
  1. {@link NetworkParameters#getId()}
    2. + *
  2. Legacy vs. Segwit
    3. + *
  3. (Legacy only) Version byte
    4. + *
  4. remaining {@code bytes}
    5. + *
+ *

+ * Implementations may use {@code compareAddressPartial} for tests 1 and 2. + * + * @param o other {@code Address} object + * @return comparison result + */ + @Override + abstract public int compareTo(Address o); + + /** + * Comparator for the first two comparison fields in {@code Address} comparisons, see {@link Address#compareTo(Address)}. + * Used by {@link LegacyAddress#compareTo(Address)} and {@link SegwitAddress#compareTo(Address)}. + * + * @param o other {@code Address} object + * @return comparison result + */ + protected int compareAddressPartial(Address o) { + // First compare netParams + int result = this.params.getId().compareTo(o.params.getId()); + if (result != 0) return result; + + // Then compare Legacy vs Segwit + if (this instanceof LegacyAddress && o instanceof SegwitAddress) { + return -1; // Legacy addresses (starting with 1 or 3) come before Segwit addresses. + } else if (this instanceof SegwitAddress && o instanceof LegacyAddress) { + return 1; + } else { + // If both are the same type, then compareTo for that type will finish the comparison + return 0; + } + } } diff --git a/core/src/main/java/org/bitcoinj/core/LegacyAddress.java b/core/src/main/java/org/bitcoinj/core/LegacyAddress.java index cf4049e01..9b6180652 100644 --- a/core/src/main/java/org/bitcoinj/core/LegacyAddress.java +++ b/core/src/main/java/org/bitcoinj/core/LegacyAddress.java @@ -23,6 +23,7 @@ import java.util.Objects; import javax.annotation.Nullable; +import com.google.common.primitives.UnsignedBytes; import org.bitcoinj.params.Networks; import org.bitcoinj.script.Script.ScriptType; @@ -213,4 +214,20 @@ public class LegacyAddress extends Address { public LegacyAddress clone() throws CloneNotSupportedException { return (LegacyAddress) super.clone(); } + + /** + * {@inheritDoc} + * + * @param o other {@code Address} object + * @return comparison result + */ + @Override + public int compareTo(Address o) { + int result = compareAddressPartial(o); + if (result != 0) return result; + + // Compare version byte and finally the {@code bytes} field itself + result = Integer.compare(getVersion(), ((LegacyAddress) o).getVersion()); + return result != 0 ? result : UnsignedBytes.lexicographicalComparator().compare(this.bytes, o.bytes); + } } diff --git a/core/src/main/java/org/bitcoinj/core/PrefixedChecksummedBytes.java b/core/src/main/java/org/bitcoinj/core/PrefixedChecksummedBytes.java index 2bc7eaece..67e191c4c 100644 --- a/core/src/main/java/org/bitcoinj/core/PrefixedChecksummedBytes.java +++ b/core/src/main/java/org/bitcoinj/core/PrefixedChecksummedBytes.java @@ -25,7 +25,6 @@ import java.lang.reflect.Field; import java.util.Arrays; import java.util.Objects; -import com.google.common.primitives.UnsignedBytes; import static com.google.common.base.Preconditions.checkNotNull; /** @@ -41,7 +40,7 @@ import static com.google.common.base.Preconditions.checkNotNull; * keys exported using Bitcoin Core's dumpprivkey command. *

*/ -public abstract class PrefixedChecksummedBytes implements Serializable, Cloneable, Comparable { +public abstract class PrefixedChecksummedBytes implements Serializable, Cloneable { protected final transient NetworkParameters params; protected final byte[] bytes; @@ -80,15 +79,6 @@ public abstract class PrefixedChecksummedBytes implements Serializable, Cloneabl return (PrefixedChecksummedBytes) super.clone(); } - /** - * This implementation uses an optimized Google Guava method to compare {@code bytes}. - */ - @Override - public int compareTo(PrefixedChecksummedBytes o) { - int result = this.params.getId().compareTo(o.params.getId()); - return result != 0 ? result : UnsignedBytes.lexicographicalComparator().compare(this.bytes, o.bytes); - } - // Java serialization private void writeObject(ObjectOutputStream out) throws IOException { diff --git a/core/src/main/java/org/bitcoinj/core/SegwitAddress.java b/core/src/main/java/org/bitcoinj/core/SegwitAddress.java index 8a494e98b..835228c2a 100644 --- a/core/src/main/java/org/bitcoinj/core/SegwitAddress.java +++ b/core/src/main/java/org/bitcoinj/core/SegwitAddress.java @@ -23,6 +23,7 @@ import java.io.ByteArrayOutputStream; import javax.annotation.Nullable; +import com.google.common.primitives.UnsignedBytes; import org.bitcoinj.params.Networks; import org.bitcoinj.script.Script; @@ -246,4 +247,19 @@ public class SegwitAddress extends Address { } return out.toByteArray(); } + + /** + * {@inheritDoc} + * + * @param o other {@code Address} object + * @return comparison result + */ + @Override + public int compareTo(Address o) { + int result = compareAddressPartial(o); + if (result != 0) return result; + + // Compare the bytes + return UnsignedBytes.lexicographicalComparator().compare(this.bytes, o.bytes); + } } diff --git a/core/src/test/java/org/bitcoinj/core/LegacyAddressTest.java b/core/src/test/java/org/bitcoinj/core/LegacyAddressTest.java index b41b380ec..8cfbcdaee 100644 --- a/core/src/test/java/org/bitcoinj/core/LegacyAddressTest.java +++ b/core/src/test/java/org/bitcoinj/core/LegacyAddressTest.java @@ -247,6 +247,16 @@ public class LegacyAddressTest { assertTrue(result > 0); } + @Test + public void comparisonNotEquals() { + // These addresses only differ by version byte + LegacyAddress a = LegacyAddress.fromBase58(MAINNET, "14wivxvNTv9THhewPotsooizZawaWbEKE2"); + LegacyAddress b = LegacyAddress.fromBase58(MAINNET, "35djrWQp1pTqNsMNWuZUES5vi7EJ74m9Eh"); + + int result = a.compareTo(b); + assertTrue(result != 0); + } + @Test public void comparisonBytesVsString() throws Exception { BufferedReader dataSetReader = new BufferedReader( diff --git a/core/src/test/java/org/bitcoinj/core/PrefixedChecksummedBytesTest.java b/core/src/test/java/org/bitcoinj/core/PrefixedChecksummedBytesTest.java index ca0998ddd..f511bbbb5 100644 --- a/core/src/test/java/org/bitcoinj/core/PrefixedChecksummedBytesTest.java +++ b/core/src/test/java/org/bitcoinj/core/PrefixedChecksummedBytesTest.java @@ -93,6 +93,7 @@ public class PrefixedChecksummedBytesTest { PrefixedChecksummedBytes a = new PrefixedChecksummedBytesToTest(params, HEX.decode("fda79a24e50ff70ff42f7d89585da5bd19d9e5cc")); PrefixedChecksummedBytes b = a.clone(); - assertEquals(0, a.compareTo(b)); + assertNotSame(a, b); + assertEquals(a, b); } }