interledger4j / ilpv4-connector Goto Github PK

See, for example, AbstractAccountBalanceSettings.

Note, this will likely affect the ITs, so we will need to see if this change is compatible without too much rewiring of those tests.

Use the shh for all encrypted values in properties files (for now).

In the updateBalanceForFulfill.lua script, we should honor the maxBalance setting for the account.

In the DefaultPacketSwitchFilterChain, the AccountSettings has already been loaded. Thus, this object should be passed into the NextHopPacketMapper to avoid a redundant Datastore load.

• Allow multiple sources-of-truth for FX (see discussion here)

• Make it easier to plugin new CurrencyProviders (there's some OK support for this now, but only CryptoCompare is supported at present).

See https://github.com/Kava-Labs/crypto-rate-utils

Spring security seems to be adding around 100ms of latency on each requests. While this is ok for the admin API, this is unacceptable for the /ilp endpoint. We should remove SpringSecurity as a dependency of that endpoint, if possible.

Note that this depends on the token framework we ultimately decide upon. Even so, if we keep JWT around, a simple JWT filter would likely be much faster than whatever spring-security is doing.

See https://github.com/tommybrettschneider/pinterest-boot for an example.

Currently, pinging doesn't cost a connector any money on any actual account. This is possibly a problem with the balance-tracking logic. So if a connector pings Alice using any particular account, Alice will receive money as part of the ping, but the source account on the Connector will not be charged because the ping protocol is engaged before the actual packet-switch and other filter logic.

This is only an issue from Connector-initiated ping requests, so we should basically just be sure that the Connector itself is not able to ping, but instead only an account can be the source of a ping packet.

This will likely affect the ilpv4 apis...

Need a swagger definition for the Admin API, plus a published location.

If an Account has CCP disabled (i.e., receiveRoute=false) and a peer attempts to send a RouteUpdate, there will be a hefty logging emission:

java.lang.RuntimeException: No tracked RoutableAccount found: `emmett-xrp`
	at com.sappenin.interledger.ilpv4.connector.packetswitch.filters.PeerProtocolPacketFilter.lambda$handlePeerRouting$2(PeerProtocolPacketFilter.java:187) ~[classes/:na]
	at java.util.Optional.orElseThrow(Optional.java:290) ~[na:1.8.0_171]
	at com.sappenin.interledger.ilpv4.connector.packetswitch.filters.PeerProtocolPacketFilter.handlePeerRouting(PeerProtocolPacketFilter.java:186) ~[classes/:na]
	at com.sappenin.interledger.ilpv4.connector.packetswitch.filters.PeerProtocolPacketFilter.doFilter(PeerProtocolPacketFilter.java:89) ~[classes/:na]
	at com.sappenin.interledger.ilpv4.connector.packetswitch.filters.DefaultPacketSwitchFilterChain.doFilter(DefaultPacketSwitchFilterChain.java:60) ~[classes/:na]
	at com.sappenin.interledger.ilpv4.connector.packetswitch.filters.ValidateFulfillmentPacketFilter.doFilter(ValidateFulfillmentPacketFilter.java:27) ~[classes/:na]
	at com.sappenin.interledger.ilpv4.connector.packetswitch.filters.DefaultPacketSwitchFilterChain.doFilter(DefaultPacketSwitchFilterChain.java:60) ~[classes/:na]
	at com.sappenin.interledger.ilpv4.connector.packetswitch.filters.BalanceIlpPacketFilter.doFilter(BalanceIlpPacketFilter.java:47) ~[classes/:na]
	at com.sappenin.interledger.ilpv4.connector.packetswitch.filters.DefaultPacketSwitchFilterChain.doFilter(DefaultPacketSwitchFilterChain.java:60) ~[classes/:na]
	at com.sappenin.interledger.ilpv4.connector.packetswitch.filters.MaxPacketAmountFilter.lambda$doFilter$3(MaxPacketAmountFilter.java:57) ~[classes/:na]
	at java.util.Optional.orElseGet(Optional.java:267) ~[na:1.8.0_171]
	at com.sappenin.interledger.ilpv4.connector.packetswitch.filters.MaxPacketAmountFilter.doFilter(MaxPacketAmountFilter.java:57) ~[classes/:na]
	at com.sappenin.interledger.ilpv4.connector.packetswitch.filters.DefaultPacketSwitchFilterChain.doFilter(DefaultPacketSwitchFilterChain.java:60) ~[classes/:na]
	at com.sappenin.interledger.ilpv4.connector.packetswitch.filters.ExpiryPacketFilter.lambda$doFilter$0(ExpiryPacketFilter.java:44) ~[classes/:na]
	at java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1590) ~[na:1.8.0_171]
	at java.util.concurrent.CompletableFuture$AsyncSupply.exec(CompletableFuture.java:1582) ~[na:1.8.0_171]
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289) ~[na:1.8.0_171]
	at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056) ~[na:1.8.0_171]
	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692) ~[na:1.8.0_171]
	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157) ~[na:1.8.0_171]

Instead, we should reduce this to just a WARN.

Throughout the code there are number of TODOs. Eventually we need to rectify those, but in the meantime we need to capture issues for these, and also fix them.

See https://github.com/sappenin/java-ilpv4-connector/pull/154/files#diff-59870e107b3e09970601e65f37472a97

Currently, the JSON looks like this:

  "isSendRoutes": false,
  "isReceiveRoutes": false

but should be:

"sendRoutes": false,
"receiveRoutes": false

Same for all other boolean properties.

Remove AppengineEnvPostProcessor. See deprecation note in there. Also remove the SPI that specifies this.

Basically, encrypted values should not be decrypted at server startup, but should instead be decrypted on an as-needed basis, and then discarded.

Remove basic-auth from the security config, and instead utilize OAuth2 using JWT tokens.

See Stats TODOs in OutgoingBalanceLinkFilter and BalanceIlpPacketFilter. Also, BalanceTracker should probably log stats.

See JS impl as well.

We should track the following things via Prometheus:

• Caffeine.

These stats for tracking packets and balances

• public incomingPackets: AccountCounter
• public outgoingPackets: AccountCounter
• public incomingSettlements: AccountGauge
• public outgoingSettlements: AccountGauge
• public rateLimitedPackets: AccountCounter

Make it easier for operators to plugin their own FX infrastructure as in instance of ExchangeRateProvider.

The filter-chain is run on every packet, and accepts an instance of AccountSettings. This value is sourced from the databstore as an AccountSettingsEntity, but the information in this object changes only rarely, so we want to cache this value so that it's not reloaded on every packet processed.

Initially, it was thought that Hibernate second-level cache could work here, however, this was not implemented for two reasons: first, configuring Ehcache with Spring Boot was somewhat opaque (e.g., it's unclear how Spring-Cache and Hibnerate work together, and getting dependencies just-right took a lot of effort). Second, and more importantly, Caffeine seems to provide much better performance than ehcache and this particular code-path is highly performance sensitive. Finally, because AccountSettings rarely change, and caching here would require a distributed cache (adding more complexity for HA environments), it was decided to only cache in the FilterChain, and refresh from the database every 15 seconds.

In theory this provides a nice balance between not waiting too long for a particular AccountSettings change from the DB to percolate to a multi-node cluster, while at the same time not impacting performance of the packet pipeline (processing tens of thousands of packets-per-second should not be impeded by a single DB load every 15 seconds).

See #168 for the follow-on task to make these values configurable.

See main pom.xml file.

Currently this would affect the following:

• ChildAccountPaymentRouter.java
• DefaultILPv4PacketSwitch
• FXCache (see PR here)

But we should search for others to be sure there aren't new caches introduced since this ticket was created.

Also, then naming convention for all of these properties should be as follows: interledger.connector.cache.{cacheName}.{cacheFeature}

For example, interledger.connector.cache.fx.ttl

Once the Connector Auth model is decided upon, cleanup defaultJwtTokenIssuer.

Currently, the packet-switch filters conflate a number of different purposes.

Sometimes, we want to filter incoming Prepare packets before they make it into the PacketSwitch (e.g., ExpiryPacketFilter, AllowedDestinationPacketFilter, and MaxPacketAmountFilter).

Other filters actually do the inverse (validate an incoming response Fulfillment like ValidateFulfillmentPacketFilter).

Finally, there are cross-cutting filters like BalanceIlpPacketFilter that do things on both the incoming (Prepare) phase, outgoing, and return path.

Replicate the quick return logic found in the Rust implementation: interledger/interledger-rs#92

The specific code in Rust is here (https://github.com/interledger-rs/interledger-rs/pull/90/files#diff-d8b400d83a08276fe9aa7835652c6a4dR114). To pull this off in Java, we would want to execute the BalanceTracker update in a separate thread.

Before we implement this ticket, however, we should create a performance test to see what impact doing balance updates in a separate thread has under load. This is because I think the spirit of this ticket is just that we should return the fulfill/reject regardless of what happens to our balance update. Generally, if the balancetracker is just down, the except will be caught and the packet returned regardless. So, the real value in implementing this ticket would be latency improvements in packet responses and making sure we respond with a packet in a timely manner even if the balance tracker is latent or hangs.

1. CCP Sender & Receiver should detect account changes (e.g., send/receive = false) and react accordingly.
2. When a new account is added to the system, we need to enable all nodes in a cluster to detect this, if peering is enabled. The correct solution here is to probably allow the routing service to periodically poll the datastore and determine if any new Peer accounts have been added.
3. We might also consider a new type of local-Event (i.e., AccountCreatedEvent) that could trigger this behavior, but because this event would be local to only a single JVM, it wouldn't help with other nodes potentially in a cluster.

Consider where ConnectorProperties should live (in server or in core), and then condense it into ConfigConstants`.

• Per-account circuit breaker (CircuitBreaker)
• Packetswitch (Timelimiter)
• Incoming Packet Limiter (RateLimiter)
• Incoming Money Limiter (RateLimiter)

Bulkhead?
More Info:

• https://medium.com/@NetflixTechBlog/performance-under-load-3e6fa9a60581
• https://github.com/resilience4j/resilience4j
• https://github.com/Netflix/concurrency-limits

We need a unique constraint on the accountId column in the database. Also, we need unit-test coverage around the case where we add an account that already exists.

When unmarhsalling JSON to something like AccountSettings, the ObjectMapper should not fail on unexpected attributes in the JSON. For example, add something like...

{
...
"__description": "The account description"
...
}

...and the API will reject the request:

For example:

"title": "Bad Request",
    "status": "400",
    "detail": "JSON parse error: Unrecognized field \"__description\" (class org.interledger.connector.accounts.ImmutableAccountSettings$Json)

Caffeine is the sequel to Guava, designed by the creators of Guava. It's much more performant than Guava, too (See https://github.com/ben-manes/caffeine/wiki/Benchmarks)

Enable https://github.com/marketplace/codecov in the build system.

This PR clarifies the Auth model for ILP-over-HTTP. The changes in this PR need to be implemented.

See DefaultNextHopPacketMapper#determineDestinationExpiresAt.

Need to make minMessageWindow and maxHoldTime configurable per account. See https://github.com/interledgerjs/ilp-connector#minmessagewindow

Also, align expiry filter timeout with these

For packet processing, we should use long and treat it as an unsigned long.

However, for other objects (e.g.,AccountBalance and BalanceTracker.getBalance) that are not in the Prepare/Fulfill flow, we should use BigInteger in order to support large negative balances that might overflow the capacity of an unsigned long.

We should have a conditional on the Admin API controllers such that if something like ilpv4.connector.admin_api is not set to true, then the Admin API is not enabled.

This will help disambiguate from the netBalance.

( ) Enable liquibase
( ) Test with GCP.

We can likely just have a single set of accounts (db-backed, ideally) and rely upon the Link mechanism to determine if the Link is eligible for sending.

When the admin account tries to create a new account using an accountId that already exists, the condition should be detected and returned as a 400 response.

Currently, this happens instead:

{
    "title": "Internal Server Error",
    "status": "500",
    "detail": "could not execute statement; SQL [n/a]; constraint [\"PUBLIC.UK_M5MSKR3IYTI61BOJK2MLI57PU_INDEX_1 ON PUBLIC.ACCOUNT_SETTINGS(NATURAL_ID) VALUES 1\"; SQL statement:\ninsert into account_settings (account_relationship, asset_code, asset_scale, max_balance, min_balance, settle_threshold, settle_to, connection_initiator, custom_settings, description, ilp_addr_segment, internal, link_type, max_packet_amt, natural_id, max_packets_per_sec, receive_routes, send_routes, id) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) [23505-199]]; nested exception is org.hibernate.exception.ConstraintViolationException: could not execute statement"
}

Previously there was a concept of an "internal" route whereby there was an extra routing table used to lookup routes that should only be viewable by the Connector, sort of like a private network space.

However, the combo of the rules in InterledgerAddressUtils and the local/forwarding routing table defined here eliminates the need of this concept, so it should be removed.

These concepts are no longer used, so we should remove them from all places in the code once we implement incoming connection support, such as from a BTP child connection.

Also, remove _AccountProviderId

Accounts are now tracked via the AccountRepository, so we should remove all accounts from YAML files.

Currently, the admin is unable to delete an account from the API.

• Update BLAST Peer topology to include 3 Connectors.
• Update BLAST Parent/Child topology to include 3 Connectors.