We leave a TODO that should be addressed after a discussion at the spec
meeting. For now, having the incorrect TLV type is not a problem since
this ChannelUpdate2 type is not used in production.
In preparation for adding a NodeAnnouncement2 struct along with a
NodeAnnouncement interface, this commit renames the existing
NodeAnnouncment struct to NodeAnnouncement1.
Add a new OutPoint type that wraps wire.OutPoint and provides TLV
encoding/decoding capabilities through the tlv.RecordProducer interface.
This enables OutPoint to be used in TLV streams of messages.
The SQL* helpers are meant to always set the `Valid` field of the
sql.Null* type to true. Otherwise they cannot be used to set a valid,
empty field. However, we dont want to break the behaviour of the
existing SQLStr helper and so this commit adds a new helper with the
desired functionality.
In this commit, we take advantage of the graph SQL migration and use it
to also extract DNS addresses from the opaque address type. We use
opaque addresses to store addresses that we dont understand yet. We
recently added logic for DNS addresses and so we may have persisted node
announcements that have DNS addresses but we would currently have them
stored under the opaque address type. So we use this migration to see if
we can extract such addresses.
A few decisions were made here:
1) If multiple DNS addressees are extracted, this is ok and we continue
to migrate the node even though this is actually invalid at a
protocol level. We will currently check (at a higher level) that a node
announcement only has 1 DNS address in it before we broadcast it though.
2) If an invalid DNS address is encountered (so we hit the DNS type
descriptor but then the rest of the DNS address payload is invalid
and cannot be parsed into the expected hostname:port, then we skip
migrating the node completely.
In this commit, the logic in ReadElements that is used to read a single
address descriptor from a io.Reader is separated out into a new function
so that the logic can be re-used elsewhere.
The first byte of an opaque addr must be one that we dont understand
yet. We do this update in preparation for doing an on-the-fly parse of
persisted opaque addrs to see if they contain addrs that we now support.
For this to work, the first byte cant be 0x01 since this maps to a known
address.
We may have already persisted node announcements that have multiple DNS
addresses since we may have received them before updating our code to
check for this. So here we just make sure not to send these on to our
peers.
Check that the node ann doesnt contain more than 1 DNS addr.
This will ensure that we now start rejecting new node announcements
with multiple DNS addrs since this check is called in the gossiper
before persisting a node ann to our local graph.
It also validates the DNS fields according to BOLT #7 specs.
PureTLVMessage describes an LN message that is a pure TLV stream. If the
message includes a signature, it will sign all the TLV records in the
inclusive ranges: 0 to 159 and 1000000000 to 2999999999.
A comprehensive test is added that shows how two versions of the same
message remain forward compatible.
This commit simplifies insertChanEdgePolicyMig. Much of the logic can be
removed given that this method is only used in the context of the graph
SQL migration.
This should improve the performance of the migration quite a lot since
it removes the extra GetChannelAndNodesBySCID call.
Finally, we make the channel-policy part of the SQL migration idempotent
by adding a migration-only policy insert query which will not error out
if the policy already exists and does not have a timestamp that is newer
than the existing records timestamp. To keep the commit simple, a
insertChanEdgePolicyMig function is added which is basically identical
to the updateChanEdgePolicy function except for the fact that it uses
the newly added query. In the next commit, it will be simplified even
more.
In this commit, we make the channel part of the graph SQL migration
idempotent (retry-safe!). We do this by adding a migration-only channel
insert query that will not error out if a the query is called and a
chanenl with the given scid&version already exists. We also ensure that
errors are not thrown if existing channel features & extra types are
re-added.
There is no need to use the "collect-then-update" pattern for node
insertion during the SQL migration since if we do have any previously
persisted data for the node and happen to re-run the insertion for that
node, the data will be exactly the same. So we can make use of "On
conflict, no nothing" here too.
In this commit, the graph SQL migration is updated so that the node
migration step is retry-safe. This is done by using migration specific
logic & queries that do not use the same node-update-constraint as the
normal node upsert logic. For normal "run-time" logic, we always expect
a node update to have a newer timestamp than any previously stored one.
But for the migration, we will only ever be dealing with a single
announcement for a given node & to make things retry-safe, we dont want
the query to error if we re-insert the exact same node.
In preparation for handling retries on the source DB side, we thread
through the `reset` call-backs properly so that we can reset appropriate
variables.
In preparation for making the channel & policy migration logic
idempotent in a step-by-step manner, we add a test here that only tests
the migration of channels _without_ policies so that we can first focus
on just making the channel migration idempotent.
Currently, the graph SQL migration is not retry safe. Meaning that if
the source DB exeutes a retry under the hood, this could result in the
migration failing. In preparation for fixing this, we adust the
migration test accordingly.
This will help us test idempotency later on, but it also ensures that
TestMigrateGraphToSQL is properly testing writes to the
graph_channel_policy_extra_types table.
Finally, we update the migrateZombieIndex function to use batch
validation just like was done in the previous commits. Here, we
additionally make sure to validate the entire zombie index entry and not
just the SCID.
As was done in the previous commits for nodes & channels, we update the
migrateClosedSCIDIndex function here so that it validates migrated
entries in batches rather than one-by-one.
As was done in the previous commits for nodes & channels, we update the
migratePruneLog function here so that it validates migrated entries in
batches rather than one-by-one.
Restructue the `migrateChannelsAndPolicies` function so that it does the
validation of migrated channels and policies in batches. So instead of
fetching channel and its policies individually after migrating it, we
wait for a minimum batch size to be reached and then validate a batch of
them together. This lets us make way fewer DB round trips.
Restructue the `migrateNodes` function so that it does the validation of
migrated nodes in batches. So instead of fetching each node individually
after migrating it, we wait for a minimum batch size to be reached and
then validate a batch of nodes together. This lets us make way fewer DB
round trips.
We do this so that this lookup is only done in the situation it is
actually needed. During a migration, we dont need to special case this
AlreadyExists error since we will terminate the transaction either way.
So there is no need for the extra lookup during the migration.
A timing analysis showed that this query was significantly impacting the
performance of the migration when run with a postgres backend.
This commit adds a BenchmarkFindOptimalSQLQueryConfig test in the
graph/db package which runs ForEachNode and ForEachChannel queries
against a local backend using various different values for the sql
QueryConfig struct. This is done to determine good default values to
use for the config options for sqlite vs postgres.
Now that the SQL query config values are configurable, we add some
validation to make sure that the user doesnt set a max batch size that
is larger than the limits for sqlite/postgres that have been determined
by the TestSQLSliceQueries test.
Here, we make the sql query option params (batch size and pagination
size) configurable. The defaults for SQLite vs Postgres are still the
same but will be changed in an upcoming commit.
The following performance gains were measured using the new benchmark
test.
```
name old time/op new time/op delta
ChanUpdatesInHorizon-native-sqlite-10 18.5s ± 3% 2.0s ± 5% -89.11% (p=0.000 n=9+9)
ChanUpdatesInHorizon-native-postgres-10 59.0s ± 3% 0.8s ±10% -98.65% (p=0.000 n=10+9)
```
Let the helper method only take the params it needs so that we dont need
to construct an entire models.ChannelEdgeInfo object to pass to it. This
will be useful later on.
Here we adjust the ForEachNodeCached graph DB method to pass in a node's
addresses into the provided call-back if requested. This will allow us
to improve the performance of node/channel iteration in the autopilot
subserver.
In preparation for removing the ForEachNode/ForEachChannel call pattern,
we refactor the autopilot subserver by simplifying the `ChannelEdge`
such that it no longer returns the `Node` type which has a
`ForEachChannel` method on it. The aim is to completely remove the
`ForEachChannel` usage.
Previously, ForEachNodeCached would batch fetch node _feature_ data but
would still fetch the channel set of each node in a node-by-node fashion
which is not ideal. So this commit updates this method to make use of
the new sqldb.ExecuteCollectAndBatchWithSharedDataQuery helper. It lets
us batch load channel data for a range of node IDs.
This _greatly_ improves the performance of the method.
In this commit, we update the SQLStore's ForEachNodeCacheable and
ForEachNodeCached methods to use batch collection for node feature bits.
This results in the following performance gains:
```
name old time/op new time/op delta
ForEachNodeCacheable-native-sqlite-10 184ms ± 2% 145ms ±10% -21.45% (p=0.000 n=10+10)
ForEachNodeCacheable-native-postgres-10 697ms ± 8% 51ms ± 4% -92.68% (p=0.000 n=9+10)
```
In this commit we add a new helper method in the sqldb package:
ExecuteCollectAndBatchWithSharedDataQuery. This can be used to paginate
through items in the database while at the same time performing batch
data collection for those items.
In this commit, we add a makefile helper that can be used to add the
"-short" flag to the "go test" command when running unit tests via "make
unit ...".
Tests can then be expanded with a `testing.Short()` check to potentially
skip the test in short mode. This is useful for if a dev wants to
quickly run most of the tests in a package but would like to opt out of
running the longer form tests such as stress tests or rapid generation
tests.
This commit adds this check to two graph/db tests.
Elle Mouton