Skip to content
This repository has been archived by the owner on Jul 20, 2021. It is now read-only.

support a 'notificator' module which allows you to subscribe to certain events #408

Closed
GlenDC opened this issue Jun 26, 2018 · 17 comments
Closed
Milestone

Comments

@GlenDC
Copy link
Contributor

GlenDC commented Jun 26, 2018

Initially the only event we would support is changes to a given address (unlockhash), as is requested by the motivating User-Facing issue: threefoldfoundation/tfchain#145

Through websockets we should expose a new protocol using JSON-RPC, which allows users to subscribe to a certain event "stream" and receive notifications for the desired subscriptions when needed. This to prevent the need for a client to having to randomly poll the server, generating traffic for no good reason, and instead receive the desired updates when they happen, without much overhead.

Subscribing to an address will be the first use-case, such that a light client could subscribe to a the address(es) it owns, and receive (un)spend outputs as they happen.

@GlenDC GlenDC added this to the 1.0.8 milestone Jun 26, 2018
@GlenDC GlenDC assigned GlenDC and unassigned GlenDC Jun 26, 2018
@GlenDC
Copy link
Contributor Author

GlenDC commented Jun 26, 2018

I thing it makes sense to only notify about relevant transactions when they actually enter the consensus, and thus are part of a created block. Others might however have a different opinion on this. Perhaps we could support both separately, or do return them within a single stream by making the distinction clear with a "confirmed" boolean property.

@LeeSmet
Copy link
Collaborator

LeeSmet commented Jun 26, 2018

Since websockets will be used, will these be exposed on the same address / port as the api (in which case it will also need to be proxied through caddy if it needs to be publicly available), or will a dedicated ip/port be used?

Furthermore, it could be made so that only the consensus set is required, and if a transactionpool is enabled, additional updates about unconfimed transactions might be sent

@GlenDC
Copy link
Contributor Author

GlenDC commented Jun 27, 2018

will a dedicated ip/port be used?

I would suggest to use a dedicated port.
Go-Ethereum does this for example as well for its websocket support.

Furthermore, it could be made so that only the consensus set is required
and if a transactionpool is enabled, additional updates about unconfimed transactions might be sent

Agreed. But even than we should make it clear in our notifications whether a transaction is confirmed or not.

@GlenDC
Copy link
Contributor Author

GlenDC commented Jun 27, 2018

This also makes it clear to me that we should also notify the address stream about reversed transactions.

So we could have 3 types of transaction notifications:

  1. unconfirmed (accepted in transaction pool)
  2. confirmed (applied in consensus)
  3. reverted (or however you want to call it)

@LeeSmet
Copy link
Collaborator

LeeSmet commented Jun 27, 2018

Agreed. But even than we should make it clear in our notifications whether a transaction is confirmed or not.

Doesn't seem to hard, if the message we send just has a type field (or similar), simply setting it to the right value based on what happened. Then it just becomes a case of properly documenting the possible values of the type enum

@LeeSmet
Copy link
Collaborator

LeeSmet commented Jun 27, 2018

Also something you possibly might want to subscribe to is new blocks, since this can be useful for "confirmed" transactions to know how safe it is to trust them, i.e. a transaction which is confirmed in the previous block could pretty easily be reverted while a txn confirmed 10 blocks prior is less likely to revert.

@GlenDC
Copy link
Contributor Author

GlenDC commented Jun 27, 2018

Also something you possibly might want to subscribe to is new blocks, since this can be useful for "confirmed" transactions to know how safe it is to trust them

Perhaps, yes. But not sure if we want to do this initially. Or if we do, not sure if we want to send the entire block, or only the headers. But not sure how do-able that is, given that our codebase is not really prepared for partial merkle tree computations as far as I understood from @robvanmieghem.

@LeeSmet
Copy link
Collaborator

LeeSmet commented Jun 27, 2018

Sending only the headers is not a problem, since I would imagine this notifier module would be subscribed to the consensus set, thus meaning it gets the consensus set updates in real time, which is a list of reverted an applied blocks, so the header is already known as it is calculated by the cs first before being pushed to subscribed modules, right?

@LeeSmet
Copy link
Collaborator

LeeSmet commented Jun 29, 2018

Initial design:

The notifier module requires a ConsensusSet module and takes an optional TransactionPool module. The Cs will provide confirmed and rollbacked blocks (transactions), whereas the Tp will provide info about new txns waiting to be confirmed.

subscribing to an address

Either the client subscribes to the address when the websocket is created through a query parameter, which means that subscribing to an additional address will require a new websocket connection. The client can not send any commands to the server (other than the ping-pong and close messages, which are part of the protocol itself). The server does not need to implement any special kind of reading ability, and only data is send over the connection.

Or the server does implement some reading functionality. The client sends predefined json objects which tell the server that it wants to subscribe to a new address or unsubscribe from an old address. This would mean that an existing connection can be reused in case the client wishes to redefine the set of subscribed addresses. But it also means that the server needs to have a goroutine for every websocket connection with the solo purpose of reading incomming data.

processing and sending data

In order to avoid wasting a lot of CPU cycles if there are multiple connections, it seems best that the parent notifier module collects all the Cs and Tp updates, and processes them. Some way to do processing could be: split a new block into a list of ExplorerTransactions, and for every transaction, figure out all addresses somehow involved in the transaction (senders addresses, receiving addresses, ...). Then, push the list of txns and involved addresses to every connection. The connection itself maintains a list of addresses its subscribed to, and pushes only the relevant transactions to the client. The connections can either have a goroutine running dedicated to writing on the websocket and read the transactionlist from a dedicated channel (thus having a goroutine and channel open for every conneced client), or the Notifier can iterate over the list of connections, start a goroutine for each of them which takes the transactionlist, checks and sends relevant transactions, and exits again.

@robvanmieghem
Copy link
Contributor

robvanmieghem commented Jun 29, 2018

Can't we implement the electrum JSON-RPC protocol for this instead of reinventing the weel?
https://electrumx.readthedocs.io/en/latest/protocol-methods.html#blockchain-address-subscribe

@GlenDC
Copy link
Contributor Author

GlenDC commented Jun 29, 2018

Yes, @robvanmieghem is correct. This feature was inspired by Electrum, so unless you have a very good to do a different than or alternative version of the electrum protocol’s adresss subscription, I would follow this protocol as described/implemented.

@robvanmieghem
Copy link
Contributor

@robvanmieghem
Copy link
Contributor

@robvanmieghem robvanmieghem removed their assignment Jul 23, 2018
@LeeSmet
Copy link
Collaborator

LeeSmet commented Jul 25, 2018

While working on the implementation of the address subscription, I ran into the following problem:

The address status is an ordered list of transactions ID's. The transaction ID's are stored in the explorer module. If an address is updates (new block or block reverted), then the cs update is propagated to the explorer, which then updates its internal DB. So the logical step would be to have the electrum module subscribe to the explorer module, but this functionality is not possible. As a workaround, the electrum module currently subscribes to the CS, relying on the fact that the explorer subscribers first, thus it gets and processes the CS update first. So when the Electrum module receives the update, the explorer already processed it and we can look up the transactions to get the new status.

The problem is that there is no guarantee that the updates are received in order of subsription, so this is not really an exceptional solution in the final design. There are multiple ways to fix this:

  1. Create subscription functionality in the explorer so we can directly subsribe to that
  2. Maintain a DB for the electrum module as well, which stores the required data. This eliminates the need for the explorer module (technically), though use of the electrum module is kind of pointless without explorer
  3. Move the Electrum module to (a subpackage of) the explorer module.

@GlenDC GlenDC modified the milestones: 1.0.8, 1.0.9 Aug 2, 2018
@robvanmieghem
Copy link
Contributor

@LeeSmet which option did you take?

@LeeSmet
Copy link
Collaborator

LeeSmet commented Aug 8, 2018

Part of my assumption from the comment was wrong. Because we need a valid explorer module in the electrum module, the explorer MUST be initialized first. As part of its initialization, its subscribes to the consensus set. This means that ones the electrum module necessarily subscribes after the explorer, and receives its updates after the explore (as slice iteration is consistently in order over the index). So there actually is an enforced order already (although this is implicit).

I'd personally prefer to go with option 1, though there is no functional value to that right now. Maybe in the future there will be, and if so we can change it then. I created a separate issue for that should it be needed, see #433

@LeeSmet LeeSmet mentioned this issue Aug 8, 2018
@robvanmieghem robvanmieghem removed this from the 1.0.9 milestone Aug 17, 2018
@LeeSmet
Copy link
Collaborator

LeeSmet commented Nov 5, 2018

Closed by 8b4b942

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants