Fix bug with allowing duplicate signature in java SubzeroUtils #709
Conversation
ivmaykov
left a comment
ivmaykov
left a comment
There was a problem hiding this comment.
Approving with some nits, please address before merging.
| // This needs to be the same sort that ScriptBuilder.createRedeemScript does. | ||
| pubkeys.sort(ECKey.PUBKEY_COMPARATOR); | ||
| List<byte[]> sortedSigs = new ArrayList<>(); | ||
| Set<String> seenSigs = new HashSet<>(); |
There was a problem hiding this comment.
super nit: I think this would work as Set<ByteString>, and then you don't have to do the byte string -> byte array -> hex string conversion on line 156 below. Not crucial though.
| } | ||
|
|
||
| private ECKey newMockPubKey() { | ||
| return new ECKey(); |
There was a problem hiding this comment.
does this constructor generate a new random key pair? If so, it's not really a "Mock", it's a real key. So maybe call the method newRandomKeyPair() instead?
| return new ECKey(); | ||
| } | ||
|
|
||
| private Signature createValidMockSignature(byte[] hash, ECKey pubkey) { |
There was a problem hiding this comment.
naming nit: ECKey is a key pair (since it's being used to sign below), not a public key
naming nit: this is not a mock signature since nothing is being mocked. We're actually creating a real signature over the provided data with a real key. Maybe call it createValidSignature()?
| pubkeys.sort(ECKey.PUBKEY_COMPARATOR); | ||
|
|
||
| byte[] hash = generateMockHash("mock input"); | ||
| List<Signature> signatures = Arrays.asList( |
There was a problem hiding this comment.
formatting nit: fix indent
| private Signature createInvalidMockSignature() { | ||
| return Signature.newBuilder() | ||
| .setHash(ByteString.copyFrom(new byte[]{0})) | ||
| .setDer(ByteString.copyFrom(new byte[]{0})) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
naming nit: maybe call this createInvalidEmptySignature()?
Also could be interesting to have a test case where we flip a bit in a valid signature, you will probably want a separate test helper for that.
| .build(); | ||
| } | ||
|
|
||
| private byte[] generateMockHash(String inputData) { |
There was a problem hiding this comment.
naming nit: this is not a mock hash, there is no mocking going on. It's a real sha-256 hash of the input string. Maybe call it generateStringHash()?
| Signature validSig = createValidMockSignature(hash, pubkeys.get(0)); | ||
| List<Signature> signatures = Arrays.asList(validSig, validSig); |
There was a problem hiding this comment.
nit: what happens if we sign twice with the same key rather than have 2 references to the same sig object? It should fail the same way because bitcoin signatures are deterministic, right? We should probably add that test case too.
|
@andozw you will have to sign your commit with a PGP key that's registered to your GH account in order to merge (repo setting). I can help you set that up on Monday if you don't have it configured. |
andozw
force-pushed
the
seana.20241115.subzero-utils-test
branch
from
401ad6d to
98c19a6
Compare
| if (sig.hasHash()) { | ||
| if (!Arrays.equals(sig.getHash().toByteArray(), hash)) { | ||
| ByteString sigHash = sig.getHash(); | ||
| ByteString expectedHash = ByteString.copyFrom(hash); |
There was a problem hiding this comment.
super nit: we only need to create this once, before the loop.
A duplicate signature would pass the validation test. This fixes that bug and adds more test coverage for the validateAndSort method.
andozw
force-pushed
the
seana.20241115.subzero-utils-test
branch
from
98c19a6 to
0a5aea9
Compare
A duplicate signature would pass the validation test. This fixes that bug and adds more test coverage for the validateAndSort method.