Not stable Transaction verifing

[officefish]

#include <iostream>

#include  "ark/arkCrypto.h"

#include <iostream>
using namespace std;

int main()
{

 string passphrase = "any passphrase";

	Address addr = Address::fromPassphrase("any other passphrase", 0);
	string recipientId = addr.toString();
	
	size_t j = 0;
	size_t i;
	for (i = 0; i < 10; ++i) {
		Transaction transaction = Builder::buildTransfer(recipientId, 24, "vendor field", passphrase);
		string signature = transaction.sign(passphrase.c_str());
		cout << "iter"<< i <<  " transaction.verify: " << transaction.verify() << endl;
		if (transaction.verify()) {
			j++;
		}
		Serializer serialiser(transaction);
		string serialised = serialiser.serialize();
		cout << "iter" << i << " serialised: " << serialised << endl;
		Deserializer deserialiser(serialised);
		transaction = deserialiser.deserialize();
		cout << "iter" << i << " transaction.verify: " << transaction.verify() << endl << endl;
	}
	
	cout << "success verify " << j << " out of " << i << " attempts";
}

// output 4 of 10 was success

[ciband]

I am able to reproduce this and looking into it.

[sleepdefic1t]

The first one has insufficient padding in r and the second one has insufficient padding in s, meaning they are negative numbers in ASN.1 IntVar format. These test vectors are detected as non-canonical signatures by Haskoin's canonical DER parser, but since I moved to this secp256k1 library, the vectors are considered valid by the default strict parser.
@XenoG: bitcoin-core/secp256k1#341 (comment)

I presume this expectation was due to BIP66.
BIP66 is stricter than canonical DER and needed to be because most underlying ECDSA implementations used in Bitcoin land will not just parse but also pass signatures which are validly encoded invalid signatures (e.g. OpenSSL decodes like an unsigned value always). So if BIP66 had limited itself to only restricting things to canonical DER the network consensus would still depend on crazy implementation specific behavior in underlying signature libraries.
gmaxwell: bitcoin-core/secp256k1#341 (comment)

[sleepdefic1t]

@officefish @ciband

Status update:

This issue is resolved in the AIP11 branch.

#include <cstdint>

#include "identities/address.hpp"
#include "identities/keys.hpp"

#include "transactions/serializer.hpp"
#include "transactions/deserializer.hpp"
#include "transactions/transaction.hpp"

#include "transactions/builders/transfer.hpp"

#include "utils/hex.hpp"

using namespace Ark::Crypto::identities;
using namespace Ark::Crypto::transactions;

int main() {
    const auto passphrase = "this is a top secret passphrase";
    const auto senderPublicKey = Keys::fromPassphrase(passphrase).publicKey;

    const auto anotherPassphrase = "this is a top secret passphrase too";
    const auto recipientId = Address::fromPassphrase(anotherPassphrase, 30).toString();

    size_t j = 0UL;
    size_t i;
    for (i = 0UL; i < 1000UL; ++i) {
        auto transaction = builder::Transfer()
                .nonce(1ULL)
                .fee(10000000)
                .senderPublicKey(senderPublicKey.data())
                .vendorField("vendorField")
                .amount(24ULL)
                .expiration(0)
                .recipientId(recipientId)
                .sign(passphrase)
                .build();

        auto verified = transaction.verify();
        printf("\niter %zu transaction.verify: %d\n", i, verified);
        if (verified) {
            j++;
        }

        const auto serialized = Serializer::serialize(transaction.data);
        printf("iter %zu serialized: %s\n", i, BytesToHex(serialized).c_str());

        memset(&transaction.data, 0, sizeof(transaction.data));

        Deserializer::deserialize(&transaction.data, serialized);
        printf("iter %zu transaction.verify: %d\n", i, transaction.verify());
    }

    printf("\nsuccess verify %zu out of %zu\n\n", j, i);
}
}

output:

iter 0 transaction.verify: 1
iter 0 serialized: ff021e0100000000000100000000000000034151a3ec46b5670a682b0a63394f863587d1bc97483b1b6c70eb58e7f0aed19280969800000000000b76656e646f724669656c641800000000000000000000001ee04f5be8d462a0a0509a3e7b9853e1d9724bb16c3045022100cd3816e5eef9eb8b5cc136970ac8e3ba73cad82ee77439c26ddd40074b6194ab02203d629a42006e939ce39b2435db346cb6a3d27d2025188f51e5d63bdb9aa0514b
iter 0 transaction.verify: 1

...

iter 999 transaction.verify: 1
iter 999 serialized: ff021e0100000000000100000000000000034151a3ec46b5670a682b0a63394f863587d1bc97483b1b6c70eb58e7f0aed19280969800000000000b76656e646f724669656c641800000000000000000000001ee04f5be8d462a0a0509a3e7b9853e1d9724bb16c3045022100cd3816e5eef9eb8b5cc136970ac8e3ba73cad82ee77439c26ddd40074b6194ab02203d629a42006e939ce39b2435db346cb6a3d27d2025188f51e5d63bdb9aa0514b
iter 999 transaction.verify: 1

success verify 1000 out of 1000

Thank you again for finding this, @officefish 👍

[sleepdefic1t]

#198 merged to develop.

[ghost]

This issue has been closed. If you wish to re-open it please provide additional information.