This will never be reliable under high load, without making it unable
to detect real errors. But the test is useful because if we don't
have this test we'll never notice if we break the const-timedness of
our implementation.
So, move the calloc out of the test loop (which seems to make it more
reliable), and then after we've run it, check the 1-minute load
average. Too high, we don't complain about results. It's not
perfect, but it's better.
Running 100 times (-O3) serially gave 100 successes with the following results:
Constant: Within 5% 562-926(832.89+/-73)/1000 times
Non-constant: More than 5% slower 860-990(956.35+/-26)/1000 times
More importantly, if we swap the const and non-const tests, we get
the expected 100 failures:
Non-constant: Within 5% 14-79(41.17+/-14)/1000 times
Constant: More than 5% slower 44-231(111.89+/-33)/1000 times
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is the normal convention for this type; it makes using converters
a little easier. See next patch.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is more reliable under load now: shorten the times so it is
likely to run in a single timeslice, and add a nanosleep so it's
likely to be at the start of the timeslice.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is the other origin, besides `bitcoin_tx`, where we create `bitcoin_tx`
instances, so add the context as soon as possible. Sadly I can't weave the
chainparams into the deserialization code since that'd need to change all the
generated wire code as well.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
The way we build transactions, serialize them, and compute fees depends on the
chain we are working on, so let's add some context to the transactions.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Simplifying some operations, erroring in some cases and moving to global
defines for constants.
Suggested-by: Rusty Russell <@rustyrussell>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
'bip70_name' is corresponding to the 'chain' field of
the API 'getblockchaininfo'.
At the beginning of lightningd, we use the 'chain' field of 'getblockchaininfo' to check if we are on right blockchain.
We check that memcmp *isn't* constant time, but that's only true under
-O2 or above: __OPTIMIZE__ doesn't distinguish.
So we need a finer-grained test. Also reduce verbosity by default.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The chainparams are needed to know the prefixes, so instead of passing down
the testnet, we pass the entire params struct.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We were deciding whether an address is a testnet address or not in the parser,
and then checking whether it matches our expectation outside as well. This
just returns the address version instead, and still checks it against our
expectation, but without having the parser need to know about address types.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This is an intermediate step since the only difference between p2pkh and p2sh
is the argument that the parsing functions take, and parsing twice for that
reason alone is quite useless.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
* remove libbase58, use base58 from libwally
This removes libbase58 and uses libwally instead.
It allocates and then frees some memory, we may want to
add a function in wally that doesn't or override
wally_operations to use tal.
Signed-off-by: Lawrence Nahum lawrence@greenaddress.it
This fixes block parsing on testnet; specifically, non-standard tx versions.
We hit a type bug in libwally (wallt_get_secp_context()) which I had to
work around for the moment, and the updated libsecp adds an optional hash
function arg to the ECDH function.
Fixes: #2563
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Pubkeys are not not actually DER encoding, but Pieter Wuille corrected
me: it's SEC 1 documented encoding.
Results from 5 runs, min-max(mean +/- stddev):
store_load_msec,vsz_kb,store_rewrite_sec,listnodes_sec,listchannels_sec,routing_sec,peer_write_all_sec
38922-39297(39180.6+/-1.3e+02),2880728,41.040000-41.160000(41.106+/-0.05),2.270000-2.530000(2.338+/-0.097),44.570000-53.980000(49.696+/-3),32.840000-33.080000(32.95+/-0.095),43.060000-44.950000(43.696+/-0.72)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is what all of this has been working towards: ripping out the handwoven
transaction handling. By removing the custom parsing we can finally switch
over to using `wally_tx` as sole representation of transactions in
memory. The commit is a bit larger but it's mostly removing setters and old
references to the input and output fields.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
These are handled internally in the `wally_tx` and do not conform to our usual
tallocated strings that can by inspected using `tal_bytelen`, and we don't
really want to litter our code with whitelisting comments for the
`amount_sat.satoshis` access, so these just do read-only on the fly conversions.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
The `wally_tx_input`s do not keep track of their input value, which means we
need to track them ourselves if we try to sign these transactions at a later
point in time.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
These are used when grinding the feerate and signing. These are just simple
facades that keep both wally and old style transactions in sync.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
During the migration to `libwally` we want to make absolutely sure that both
transactions are generated identical, and can eventually be switched over.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We are slowly migrating towards a wally-transactions only world, but to make
this reviewable we start building both old and new style transactions in
parallel. In a second pass we'll then start removing the old ones and use
libwally only.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We set the version BIP32_VER_TEST_PRIVATE for testnet/regtest
BIP32 privkey generation with libwally-core, and set
BIP32_VER_MAIN_PRIVATE for mainnet.
For litecoin, we also set it like bitcoin else.
We need to do it in various places, but we shouldn't do it lightly:
the primitives are there to help us get overflow handling correct.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We need to still accept it when parsing the database, but this flag
should allow upgrade testing for devs building on top
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We had a bug 0ba547ee10 caused by
short_channel_id overflow. If we'd caught this, we'd have terminated
the peer instead of crashing, so add appropriate checks.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I upgraded my node with --disable-compat, and a heap of channels closed like:
CHANNELD_NORMAL:We disagree on short_channel_ids: I have 557653x0x1351, you say 557653x2373x1",
This is because the scids are strings in the databases, and it failed to parse
them properly.
Now we'll not start if that happens.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Currently only used by gossipd for channel elimination.
Also print them in canonical form (/[01]), so tests need to be
changed.
Suggested-by: @cdecker
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is prep work for when we sign htlc txs with
SIGHASH_SINGLE|SIGHASH_ANYONECANPAY.
We still deal with raw signatures for the htlc txs at the moment, since
we send them like that across the wire, and changing that was simply too
painful (for the moment?).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We currently make sure that all the bitcoin_tx input scripts are NULL
and set the input script of the input we're signing, so we can easily
reuse the tx hashing code for signature checks. This means that we
sometimes jump through hoops to make sure input scripts are NULL, and
also means that the tx can't be const.
Put more logic inside bitcoin/tx so it can simply ignore things we
don't want to hash.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
To be safe, we should never memcmp secrets. We don't do this
currently outside tests, but we're about to.
The tests to prove this as constant time are the tricky bit.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
tal_count() is used where there's a type, even if it's char or u8, and
tal_bytelen() is going to replace tal_len() for clarity: it's only needed
where a pointer is void.
We shim tal_bytelen() for now.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Really, we should have a 'struct point' since we don't use all points
as pubkeys. But this is the minimal fix to avoid type cast nastiness.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
structeq() is too dangerous: if a structure has padding, it can fail
silently.
The new ccan/structeq instead provides a macro to define foo_eq(),
which does the right thing in case of padding (which none of our
structures currently have anyway).
Upgrade ccan, and use it everywhere. Except run-peer-wire.c, which
is only testing code and can use raw memcmp(): valgrind will tell us
if padding exists.
Interestingly, we still declared short_channel_id_eq, even though
we didn't define it any more!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I leave all the now-unnecessary accessors in place to avoid churn, but
the use of bitfields has been more pain than help.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
With fallback depending on chainparams: this means the first upgrade
will be slow, but after that it'll be fast.
Fixes: #990
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
When a serialized length refers to an array of structures, the trivial
DOS prevention can be out by a factor of sizeof(serialized struct). Use
the size of the serialized structure as a multiplier to prevent this.
Transaction inputs are the motivating example, where the check is out by
a factor of ~40.
If no witnesses are present on any inputs, then extended serialisation
should not be used.
[ Amended to make adding new flags clearer in future -- RR ]
Signed-off-by: Jon Griffiths <jon_p_griffiths@yahoo.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
* Add BITCOIN_TEST_PROGRAMS to ALL_TEST_PROGRAMS
* Refactor bitcoin test make directives into its own Makefile under bitcoin/test
Signed-off-by: William Casarin <jb55@jb55.com>
The deserialization of bitcoin transactions in wire/ is rather
annoying in that we first allocate a new bitcoin_tx, then copy it's
contents onto the destination and then still carry the newly allocated
one around due to the tal-tree. This splits `pull_bitcoin_tx` into
two: one part that does the allocation and another one that proceeds
to parse.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
It's just a sha256_double, but importantly when we convert it to a
string (in type_to_string, which is used in logging) we use
bitcoin_blkid_to_hex() so it's reversed as people expect.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
trueptolemy
Rusty Russell
Karl-Johan Alm
Christian Decker
trueptolemy
lisa neigut
GreenAddress
Saibato
practicalswift
nicolas.dorier
Luca Vaccaro
Jon Griffiths
William Casarin