Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Good test vectors for signed transactions #1018

Closed
matejcik opened this issue May 26, 2020 · 4 comments · Fixed by #2075
Closed

Good test vectors for signed transactions #1018

matejcik opened this issue May 26, 2020 · 4 comments · Fixed by #2075
Assignees
Labels
code Code improvements tests Automated integration tests

Comments

@matejcik
Copy link
Contributor

Many transaction test vectors in our testsuite aren't actually valid transactions: they are spending outputs that do not exist, have mismatched amounts, or belong to different addresses.

In general, the majority of transaction test vectors only validate our implementation in core vs legacy, and check that no regressions in the signing code happened. As there is currently no good way to validate transactions against the real cryptocurrency implementation, there could be invisible serialization bugs and such.

The goal of this issue is to improve the test suite in this regard.

The Platonic ideal of a test vector is one that you can copy-paste from the test suite and spend on-chain directly. This is of course impossible.

The next best thing is validating that a Trezor-generated transaction comes out identical to a transaction that has already been accepted by the network. We have several such testcases, but we've lost many of them because, e.g., they are using invalid BIP32 paths.
It is rather difficult to generate such transactions as needed, because one needs to create live transaction to serve as inputs, a live transaction that spends them, that costs money and time and some amount of bookkeeping.

As an intermediate goal, it would be good to have a "full feature set" of transactions that have been accepted by Testnet (or the respective testnets of different altcoins). This removes the need to spend real money and limits the amount of necessary bookkeeping to "get coins from faucet, return coins to faucet".

Another option is to run a blockchain internally.

Finally, we can explore ways to take advantage of testmempoolaccept (and equivalents?), possibly coupled with reverting the node to a block at particular height, so that the UTXOs being used are unspent at that time. This would allow us to independently check the validity of the test vectors.

@matejcik matejcik added firmware tests Automated integration tests code Code improvements labels May 26, 2020
@prusnak prusnak added this to the backlog milestone Jun 7, 2020
@tsusanka tsusanka removed W? labels Feb 19, 2021
@tsusanka tsusanka removed this from the backlog milestone Oct 6, 2021
@tsusanka
Copy link
Contributor

tsusanka commented Dec 2, 2021

Please note that we now have Bitcoin regtest in trezor-user-env (trezor/trezor-user-env#99) and also a first test using this in Suite 🎉 (trezor/trezor-suite#4540). Also note that @karliatto was responsible for making the first test (in another words set up the infrastructure) and I would like QA to expand the tests furthermore after our new Head of QA starts in ~February.

@matejcik
Copy link
Contributor Author

matejcik commented Jan 12, 2022

some notes:

(a) Using regtest is impractical.

  • Everything we actually need to test, we can test with static vectors. Static vectors are also easier to work with.
  • Each developer would need a regtest instance, a rather heavy dependency.
  • Not mentioning that this is not scalably applicable for altcoins.
  • Suite will already have end-to-end tests with regtest, so big-picture we got that part covered.

(b) We are reasonably happy with the current tests. Except for the bad data, the process works pretty much as intended.

Fixing the bad data might be tedious (or we might write a supporting tool to make it easier -- note to self, libcst to the rescue?) but we expect it to be a one-time thing.


Action items:

(1) Enable checking scriptSigs scriptPubKeys for inputs being spent. This is a very small change that will identify which tests have broken data.
@andrewkozlik is on it

(2) Get a list of all available transactions:

  • for all all all... seed
  • for no passphrase, or TREZOR passphrase (there might not be anything on it, but we're using it in tests in some places so 🤷‍♀️ )
  • on Bitcoin and Testnet
  • for all account types, for all 10 accounts (presumably most will be empty)
  • separated by account type

Make a script that does this, so we can later adapt it for Bcash/Bgold/etc. if needed.

From (2) we should hopefully be able to pick appropriate inputs to fix the failing tests.

@matejcik
Copy link
Contributor Author

(c) We have two kinds of tests:

  1. verifying that the transaction + signatures are valid and acceptable by network
  2. (more common) verifying that faced with certain transaction description, Trezor behaves in an expected way: correctly identifies change, correctly validates parameters, etc.

Although nice to have, we do not need real-world transactions to test (2). Depending on which tests are failing and what data we have available, we still might go with synthetic transactions as inputs for those tests.

@andrewkozlik
Copy link
Contributor

andrewkozlik commented Jan 12, 2022

Action item 1:
See branch andrewkozlik/scriptpubkey-check, which results in 86 device tests failing: https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/1967697885. BTW, the Taproot test look fine. I will add the same change to the T1 code and also look into fixing the core unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Code improvements tests Automated integration tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants