zpay32: improve fuzz tests #7723
Conversation
morehouse
force-pushed
the
fuzz_zpay32_checksum
branch
from
9b07377
to
75e725f
Compare
|
|
||
| // Call these functions as a sanity check to make sure the | ||
| // invoice is well-formed. | ||
| _ = inv.MinFinalCLTVExpiry() |
There was a problem hiding this comment.
I think this should stay since it can let us know if the function breaks
There was a problem hiding this comment.
It seems like there's no present benefit to doing the function calls here. There's nothing in the functions that can return an error or panic, and we don't check the return values anyway.
But its easy to drop this commit if you want.
There was a problem hiding this comment.
We can get rid of it, though sometimes I find this pattern useful to see if a member function gives an unexpected side effect
zpay32/fuzz_test.go
Outdated
| // invoice is well-formed. | ||
| _ = inv.MinFinalCLTVExpiry() | ||
| _ = inv.Expiry() | ||
| _, _ = Decode(data, &chaincfg.TestNet3Params) |
There was a problem hiding this comment.
I think it would be a good idea to get the fuzzer to randomize the chaincfg
There was a problem hiding this comment.
Done in 7390e6d23b6fd8329956a278892451ea6f0edb5f.
|
I've added 7390e6d, which lets the fuzzer choose the invoice network. Note that this is a backwards-incompatible change with the existing seeds in lnd-fuzz. So we'll need to merge a PR over there at the same time we merge this PR, or else CI will break. |
|
@Crypt-iQ: review reminder |
|
can merge when tests fixed |
lightningnetwork/lnd#7723 adds a byte parameter to FuzzDecode that selects the chain to use for decoding the invoice. We add that parameter to the existing seeds so that they continue to run for TestNet3.
lightningnetwork/lnd#7723 changed FuzzEncode significantly, such that the old seeds are no longer useful even if we modified them to use the new parameters.
Generated from fuzzing with the updated FuzzDecode target from lightningnetwork/lnd#7723.
Seeds generated after the overhaul of FuzzEncode in lightningnetwork/lnd#7723.
Companion PR: lightninglabs/lnd-fuzz#11 These need to be merged together to keep tests passing. |
|
@morehouse can you rebase to re-trigger the unit tests please? I merged lightninglabs/lnd-fuzz#11. |
The fuzz tests call inv.MinFinalCLTVExpiry() and inv.Expiry() supposedly to ensure the invoice is well-formed. However, those methods can never panic or return errors and therefore provide no benefit for this purpose.
The message signer from invoice_test.go is identical to the one created in the fuzz test. We're already using the private key from invoice_test.go, so we may as well use the complete message signer for simplicity.
It is very difficult for the fuzzer to create a valid checksum for each serialized invoice, and we were therefore unable to fuzz deeper than invoice decoding. We can help the fuzzer generate valid serialized invoices by calculating and appending the checksum ourselves. We also switch to using mainnet invoices to make it easier to find valid invoices for seeding the fuzzer. We prepend the required "lnbc" prefix ourselves to further help the fuzzer generate valid invoices.
We add a parameter to select which network will be used for the fuzz tests, rather than hardcoding the network.
morehouse
force-pushed
the
fuzz_zpay32_checksum
branch
from
7390e6d
to
9200abf
Compare
|
Rebased. |
Even after 200+ CPU hours, the fuzzer was unable to generate any serialized invoices with a valid checksum (see lightninglabs/lnd-fuzz#5 (comment)). This PR helps the fuzzer out by appending the required checksum manually. As a result, the fuzz tests now have pretty decent coverage of both invoice decoding and encoding.
The PR also contains other minor improvements and simplifications of the fuzz tests.