sqlite3 was forgiving, postgres isn't, so let's make sure we use the strictest
field type possible, relaxing when rewriting.
The commit consists just of the following mapping
- INTEGER -> BIGSERIAL if it is the primary key
- INTEGER -> BIGINT if it is an amount or a reference to a primary key
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This was already done in `db_step` but `db_count_changes` and
`db_last_insert_id` also rely on the statement being executed. Furthermore we
now check that the statement was executed before freeing it, so it can't
happen that we dispose of a statement we meant to execute but forgot.
The combination of these could be used to replace the pending_statement
tracking based on lists, since we now make sure to execute all statements and
we use the memleak checker to make sure we don't keep a statement in memory.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
sqlite3 doesn't really do any validation whatsoever, and there is no
difference between 64bit and 32bit numbers. Posgtres on the other hand gets
very upset if the size doesn't match.
This commit swaps out handwavy types with the ones that should be there :-)
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This was weird right from the start, so we just split the table into integers
and blobs, so each column has a well-defined format. It is also required for
postgres not to cry about explicit casts in the `paramTypes` array.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
The first ever query to check if the version DB exists may fail. We allow
this, but we need to restart the DB transaction since postgres fails the
current transaction and rolls back any changes.
This just commits (and fails) and starts a new transaction so the rest of the
migration can continue.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Needed to change a couple of migrations. The changes are mostly innocuous:
- changing BLOB to TEXT for short_channel_ids which is the correct type
anyway, and sqlite3 treats them the same anyway.
- Use `int` for version since the byte representation is checked by postgres.
- Change anything that is INT, but will be bound to u64 to BIGINT (again
postgres checks these more carefully than sqlite3).
Two migrations were replaced with dummy values, since they are buried deep
enough, and I found no portable way of expressing `strftime()` and `INSERT OR
IGNORE`.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Using a generated identifier with filename and line proved to be brittle since
compilers assign the __LINE__ macro differently on multi-line macro
invocations.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This is dangerous but needed since postgres is not as forgiving about
unsatisfied foreign key constraints even while in a DB transaction.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We used to do some of the setup work in db.c, which is now free of any
sqlite3-specific code. In addition we also switch over to fully qualified DSNs
to specify the location of the wallet.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We're about to remove them.
Includes fix to sqlite3_bind_short_channel_id to not assume `id` is a
tal object.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now that all the users are migrated to the abstraction layer we can remove the
legacy implementation.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
It's better to let the driver decide when and how to expand. It can then
report the expanded statement back to the dispatch through the
`db_changes_add` function.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We now have a much stronger consistency check from the combination of
transaction wrapping, tal memory leak detection. Tramsaction wrapping ensures
that each statement is executed before the transaction is committed. The
commit is also driven by the `io_loop`, which means that it is no longer
possible for us to have statements outside of transactions and transactions
are guaranteed to commit at the round's end.
By adding the tal-awareness we can also get a much better indication as to
whether we have un-freed statements flying around, which we can test at the
end of the round as well.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This is likely the last part we need to completely encapsulate the part of the
sqlite3 API that we were using. Like the `db_count_changes` call I decided to
pass in the `struct db_stmt` since really they refer to the statement that was
executed and not the db.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
These are based on top of the basic column access functions, and act as a
small type-safe wrapper, that also does a bit of validation.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This has a slight side-effect of removing the actual begin and commit
statements from the `db_write` hooks, but they are mostly redundant anyway (no
harm in grouping pre-init statements into one transaction, and we know that
each post-init call is supposed to be wrapped anyway).
Signed-off-by: Christian Decker <decker.christian@gmail.com>
These are used to do one-time initializations and wait for pending statements
before closing.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
I was hoping to get rid of these by using "ON CONFLICT" upserts, however
sqlite3 only started supporting them in version 3.24.0 which is newer than
some of our deployment targets.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This is the first step towards being able to extract information from query
rows. Only the most basic types are exposed, the others will be built on top
of these primitives.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
For some of the query methods in the next step we need to have an idea of
whether the stmt was executed (db_step function) so let's track that
explicitly.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
These do not require the ability to iterate over the result, hence they can be
migrated already.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
These functions implement the lookup of the query, and the dispatch to the
DB-specific functions that do the actual heavy lifting.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
All drivers will have to reach into it, so put it in a place that is reachable
from the drivers, along with all other definitions.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This is the counterpart of the annotations we did in the last few commits. It
extracts queries, passes them through a driver-specific query rewriter and
dumps them into a driver-specific query-list, along with some metadata to
facilitate processing later on. The generated query list is then registered as
a `db_config` and will be loaded by the driver upon instantiation.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We will soon generalize the DB, so directly reaching into the `struct db`
instance to talk to the sqlite3 connection is bad anyway. This increases
flexibility and allows us to tailor the actual implementation to the
underlying DB.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
`db_select_prepare` was prepending the "SELECT" part in an attempt to limit
its use to read-only statements. This is leads to the queries in the code not
actually being well-formed, which we'll need in a later commit, and was also
resulting in extra allocations. This switches the behavior to just enforce a
"SELECT" prefix being present which allows us to have well-formed queries in
the code again and avoids the extra allocation.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We need to have full DB queries that can be extracted at compile time later in
order to be able to rewrite them in other SQL dialects. In addition we had a
bit of unnecessary code-duplication in db_select and db_select_prepare. Now
the former uses the latter internally.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Dumb programs which have a --daemon option call fork() early. This is
terrible UX since startup errors get lost: the program exits with
"success" immediately then you discover via the logs that it didn't
start at all.
However, forking late introduced a heap of problems with changing
pids. Instead, fork early but keep stderr and the parent around: if
we fail early on, the parent fails with us. We release our parent
with an explicit action just before the main loop.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
These are generalized from our internal implementations.
The main difference is that 'struct json_escaped' is now 'struct
json_escape', so we replace that immediately.
The difference between lightningd's json-writing ringbuffer and the
more generic ccan/json_out is that the latter has a better API and
handles escaping transparently if something slips through (though
it does offer direct accessors so you can mess things up yourself!).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Mainly used to differentiate channel-related transactions from on-chain wallet
transactions. Will be used to filter `listtransaction` results and bundle
transactions that belong to the same channel.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We'd like to display the receive and resolution times in the forwardings
table. In order to remember the receive time we need to store it in the DB
along with the other information.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Node ids are pubkeys, but we only use them as pubkeys for routing and checking
gossip messages. So we're packing and unpacking them constantly, and wasting
some space and time.
This introduces a new type, explicitly the SEC1 compressed encoding
(33 bytes). We ensure its validity when we load from the db, or get it
from JSON. We still use 'struct pubkey' for peer messages, which checks
validity.
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
39475-39572(39518+/-36),2880732,41.150000-41.390000(41.298+/-0.085),2.260000-2.550000(2.336+/-0.11),44.390000-65.150000(58.648+/-7.5),32.740000-33.020000(32.89+/-0.093),44.130000-45.090000(44.566+/-0.32)
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>
wallet/test/run-wallet was failing the valgrind check; turns out
`sqlite3_expanded_sql` expects you to manage the memory of strings
it returns. from `sqlite3.h`:
** ^The string returned by sqlite3_expanded_sql(P), on the other hand,
** is obtained from [sqlite3_malloc()] and must be free by the application
** by passing it to [sqlite3_free()].
I was tempted to create a new db_select_stmt wrapper type, but that means
a lot of boilerplate around binding, which expects to work with db_prepare
*and* db_select_prepare.
This lets us clearly differentiate between db queries (which don't need to
go to a plugin) and db changes (which do).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
- Intrduce DB update `channel` values: `feerate_base` and `feerate_ppm`
- Make fist use of now context realted DB migration
- Add `struct channel` members of the same name
- Use struct values instead of config when commiting new channels
Allow a function as well as (or instead of!) an sql statement. That
will let us do things like set per-channel values to the global
defaults, for example.
Since we remove the NULL termination, the final entry is ARRAY_SIZE()-1
not ARRAY_SIZE()-2.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
More efficient to measure the ARRAY_SIZE(), which is a runtime
constant. We move it into the unit test.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Below this code appears:
if (current != orig)
db_exec(__func__, db,
"INSERT INTO db_upgrades VALUES (%i, '%s');",
orig, version());
But since the loop pre-increments current, this is always true. I wondered
why there were so many duplicates in my db_upgrades table!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This field was used by `pay` to hold the bolt11 description if the bolt11
string used `h` to hash the description (which nobody ever did). If the
`h` field wasn't present, it could contain anything, as it wasn't checked.
It's really useful to have a label for payments (eg. '1 Cuban'), but adding
yet-another option would be painful, so we simply rename 'description'
to 'label' except inside the db.
This means we need to do some tricky parameter parsing to handle array
and keyword JSON arguments, but only until we remove the old name.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Without this, there's no proof of payment, since it is the signed invoice
that make the receipt valid.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In order to avoid having to ask the HSM for public keys to
their_unilateral/to_us outputs we just store the `scriptPubkey` with the UTXO,
which can then be converted to the P2WPKH address.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
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 were not correctly allocating the `db->filename`, failing to copy the
null-terminator. This was causing and error when reopening the database after
the call to `fork()`.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Reported-by: Sean McNally <@sfmcnally>
Changelog-fixed: Fixed a crash when running in daemon-mode due to db filename overrun
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>
Christian and I both unwittingly used it in form:
*tal_arr_expand(&x) = tal(x, ...)
Since '=' isn't a sequence point, the compiler can (and does!) cache
the value of x, handing it to tal *after* tal_arr_expand() moves it
due to tal_resize().
The new version is somewhat less convenient to use, but doesn't have
this problem, since the assignment is always evaluated after the
resize.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
json_escaped.[ch], param.[ch] and jsonrpc_errors.h move from lightningd/
to common/. Tests moved too.
We add a new 'common/json_tok.[ch]' for the common parameter parsing
routines which a plugin might want, taking them out of
lightningd/json.c (which now only contains the lightningd-specific
ones).
The rest is mainly fixing up includes.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's the only user of them, and it's going to get optimized.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
gossip.pydiff --git a/common/test/run-json.c b/common/test/run-json.c
index 956fdda35..db52d6b01 100644
We do this a lot, and had boutique helpers in various places. So add
a more generic one; for convenience it returns a pointer to the new
end element.
I prefer the name tal_arr_expand to tal_arr_append, since it's up to
the caller to populate the new array entry.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's an array: we were only saving the single element; if there was more than
one changed HTLC we'd get a bad signature!
The report in #1907 is probably caused by the other side re-requesting
something we considered already finalized; to avoid this particular error,
we should set the field to NULL if there's no last_sent_commit.
I'm increasingly of the opinion we want to just save all the update
packets to the db and blast them out, instead of doing this
second-guessing dance.
Fixes: #1907
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This means we don't try to unilaterally close after a restart, *and*
we can tell onchaind to try to use the point to recover funds when the
peer unilaterally closes.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We're currently overriding fatal() with something that actually
returns, which contrasts with its declaration as NORETURN.
This breaks in the next patch which wants a real fatal() in wallet.h.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In several places we use low-level tal functions because we want the
label to be something other than the default. ccan/tal is adding
tal_*_label so replace them and shim it for now.
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>
I would have liked to make it a tal object, then we'd catch most
things with our memleak detection. However, sqlite3 doesn't seem to
allow allocator overrides.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The no-rescan change requires us to rescan one last time from the first_blocknum
of our channels (if we have any). The migrations just drop blocks that are
higher, then insert a dummy with the first_blocknum, and then clean up after
us. If we don't have any channels we don't go back at all.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
These transactions being seen on the blockchain triggered some action in
onchaind so we need to replay them when we restore the onchaind.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Currently these are either transactions we sent ourselves or transactions that
we are watching because they are part of a channel.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
So we know how much counterparty could theoretically steal from us
(msatoshi_to_us - msatoshi_to_us_min) and how much we could
theoretically steal from counterparty (msatoshi_to_us_max -
msatoshi_to_us).
For more piloting goodness.
This may be causing #1280, since with `--daemon` the DB is being reopened
without enabling the foreign key relations and hence the delete cascades.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
It would be better to give them unique values, but we don't fully support
db migrate anyway and this is simple (though they will end up using the
same key for multiple channel closes if created before this commit).
Note that even if bip32_max_index is currently unset, it defaults to 0
so it will be found.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The billboard is now far more useful to tell what's going on, and this
gets us closer to a state == owner mapping.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And now we can finally do the db upgrade to remove any OPENINGD
channels once, since we never put them back.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We use it on the secrets array for the moment, but it's also useful
for remote_shutdown_scriptpubkey, as used in the next patch.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We usually did this, but sometimes they were named after what they did,
rather than what they cleaned up.
There are still a few exceptions:
1. I didn't bother creating destroy_xxx wrappers for htable routines
which already existed.
2. Sometimes destructors really are used for side-effects (eg. to simply
mark that something was freed): these are clearer with boutique names.
3. Generally destructors are static, but they don't need to be: in some
cases we attach a destructor then remove it later, or only attach
to *some* cases. These are best with qualifiers in the destroy_<type>
name.
Suggested-by: @ZmnSCPxj
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We need these to decode any returned errors.
We remove it from struct pay_command too, and load directly from db
when we need it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We should be saving this, as it's our proof of payment. Also, we return
it if they try to pay again.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Paid invoices need to know how much was actually paid: both for the case
where no 'msatoshi' amount was specified, and for the normal case, where
clients are permitted to overpay in order to help them disguise their
payments.
While we migrate the db, we leave this field as 0 for old paid
invoices. This is unhelpful for accounting, but at least clearly
indicates what happened if we find this in the wild.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is necessary to grad the their_unilateral/to-us outputs since
they aren't being harvested by `onchaind`
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We save location where transaction was started, in case we try to nest.
There's now no error case; db_exec_mayfail() is the only one.
This means the tests need to override fatal() if they want to intercept
these errors.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
we should never be doing two startups at once, but why take chances? Plus,
we can then assert that all db calls are in transactions.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Otherwise we find ourselves outside a commitment. This is a bandaid
until we remove nested commitments again at the end of this series.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Nesting is provided by only actually performing the outermost
transaction and simulating the nested ones. This still allows us to
ensure on lower levels that we are in the context of a transaction
without having to resort to keeping explicitly track of it in the
calling code.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
In addition we also set some of the test values to a pattern instead
of just `memset`ting it to 0, which may hide some crossed lines.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We use these quite often and it is cumbersome having to do these
simple conversions inline, so just expose pseudo-sqlite3 methods to
bind and extract from/to a stmt.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Technically it's the caller that'll own the statement, but it is nice
to have db_exec_prepared dispose of it.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
