Run with mutagen on travis. #453
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TheBlueMatt
force-pushed
the
2020-01-mutagen
branch
20 times, most recently
from
f2a588c to
511cbdc
Compare
|
Okay, so if I understand mutation correctly, a good test suite should catch any mutation introduced by mutagen. How the process is going ? We add |
TheBlueMatt
force-pushed
the
2020-01-mutagen
branch
from
511cbdc to
17e2afa
Compare
Very poorly :). I had to check a bunch of functions before I found ones where our test suite caught all mutagen-default mutations :(. |
elichai
reviewed
Jan 20, 2020
| [dependencies] | ||
| bitcoin = "0.21" | ||
| bitcoin_hashes = "0.7" | ||
| secp256k1 = "0.15" | ||
| mutagen = { git = "https://github.com/TheBlueMatt/mutagen", optional = true } |
There was a problem hiding this comment.
I think you meant to put this in dev-dependencies?
There was a problem hiding this comment.
No, for some reason dev-dependencies can't be optional, and we don't want to require a nightly-only crate for any cargo test.
There was a problem hiding this comment.
right :( that's one of the oldest bugs we're still stuck with somehow (cc rust-lang/cargo#1596)
|
The feature part is fixed in master llogiq/mutagen#155 |
Previously OnionHopData contained a OnionRealm0HopData field however instead of bumping the realm number, it has been replaced with a length, used to indicte the length of a TLV-formatted object. Because a TLV-formatted hop data can contain the same information as a realm-0 hop data, we flatten the field and simply keep track of what format it was in.
Its a bit awkward to have an hmac field covering the struct that its in, and there is little difference in removing it, so just pull it out and use a [u8; 32] where we care about the hmac.
This, as it should be, restricts OnionHopData to only being able to represent valid states, while still allowing for tests to generate bogus hop data fields to test deserialization.
This also renames PendingForwardHTLCInfo to PendingHTLCInfo since it now also encompasses Pending *Received* HTLCs.
This should avoid blowing up the size of the struct when we add additional data that is only relevant for receive.
This is the first step in Base AMP support, just tracking the relevant data in internal datastructures.
We should probably do this for all values (and define a newtype for msat values), but this will do for now.
Sadly a huge diff here, but almost all of it is changing the method signatures for sending/receiving/failing HTLCs and the PaymentReceived event, which all now need to expose an Option<[u8; 32]> for the payment_secret. It doesn't yet properly fail back pending HTLCs when the full AMP payment is never received (which should result in accidental channel force-closures). Further, as sending AMP payments is not yet supported, the only test here is a simple single-path payment with a payment_secret in it.
Rather big diff, but its all mechanical and doesn't introduce any new features.
This rather dramatically changes the return type of send_payment making it much clearer when resending is safe and allowing us to return a list of Results since different paths may have different return values.
We only do this for incoming HTLCs directly as we rely on channel closure and HTLC-Timeout broadcast to fail any HTLCs which we relayed onwards where our next-hop doesn't update_fail in time.
In case of committing out-of-time outgoing HTLCs, we force ourselves to close the channel to avoid remote peer claims on a non-backed HTLC
If our counterparty burns their funds by revoking their current commitment transaction before we've sent them a new one, we'll step forward the remote commitment number. This would be otherwise fine (and may even encourage them to broadcast their revoked state(s) on chain), except that our new EnforcingChannelKeys expects us to not jump forward in time. Since it isn't too important that we punish our counterparty in such a corner-case, we opt to just close the channel in such a case and move on.
|
Nice! Glad that got upstreamed! |
TheBlueMatt
force-pushed
the
2020-01-mutagen
branch
from
2580372 to
04fdb35
Compare
Previously, in each of our fuzz tests we had a dummy test which had a hard-coded hex string which it passed into the fuzz target so that when a failing test case was found, its hex could be copied into the test and you could run cargo test to analyze the failure. However, this was somewhat unwieldy as converting large tests back and forth between hex and raw files is quite annoying. Instead, we replace each of those tests with a test in each target that looks for files in fuzz/test_cases and runs each file it finds. Since we're editing every bin target anyway, we also automate adding no_main to libfuzzer builds with #![cfg_attr].
full_stack_target found a crash where we may overflow ruring fee calculation if a transaction appears on-chain with massive value available for us to claim. Since these transactions are clearly bogus, we shouldn't allow full_stack_target to connect them, but we also improve the error generated by explicitly panicing on them.
TheBlueMatt
force-pushed
the
2020-01-mutagen
branch
from
04fdb35 to
63697a4
Compare
Sadly our test coverage isn't very good and I had to hunt for functions to mutate where we fail tests on every mutation mutagen creates, but committing the framework is a start.
TheBlueMatt
force-pushed
the
2020-01-mutagen
branch
from
63697a4 to
f2f7e70
Compare
Closed
|
Is there any want/need to get this over to GitHub Actions? |
|
Yea I think ideally we would still get this running, at least so we have the infrastructure for it and we can add it to new state machine functions. Gonna go ahead and close this, though. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sadly our test coverage isn't very good and I had to hunt for
functions to mutate where we fail tests on every mutation mutagen
creates, but committing the framework is a start.
CC #188