[Design] General architecture, features, and API.

Introduction
The hathor-wallet-service will be used as backend for all official Hathor's wallets.
This design is highly inspired in the Bitcore Wallet Service.
TODO LIST

[ ] How to handle tx with timelock and reward lock?
[ ] How to handle Authority UTXO?
[ ] NoSQL vs SQL
[ ] Describe sync between client and service
[ ] Describe sync between full node and service

TEST LIST

[ ] From Executed to Voided transactions
[ ] From Voided to Executed transactions
[ ] From Executed to Voided blocks
[ ] From Voided to Executed blocks
[ ] Authority UTXO
[ ] Token creation
[ ] Transaction timelock
[ ] Reward lock

Requirements

Support running multiple instances under a Load Balance, which will scale as more wallets are being used.

Architecture

┌──────────────┐       ┌─────────────────┐       ┌───────────────┐
│              │       │                 │       │               │
│  Full node   │──────▶│  Tx Processor   │◀─────▶│   Database    │
│              │       │                 │       │               │
└──────────────┘       └─────────────────┘       └───────────────┘
                                │                        ▲
                                │                        │
                                │                        ▼
                                │                ┌───────────────┐
                                │                │               │
                                └───────────────▶│  API Service  │
                                                 │               │
                                                 └───────────────┘

The service will be implemented using AWS services.

DynamoDB: Database
AWS Lambda: Tx Processor and API Service
AWS Gateway API: HTTP and Websocket endpoints
AWS SNS: Messaging service
AWS SQS: Queue service
AWS CodePipeline (test & continuous deploy)

Services
Tx Processor
Receive transactions/blocks from the full node and update the database. It will receive two types of messages: NEW_TX and UPDATE_TX. The latter is used to notify a change in the tx status (from executed to voided or otherwise).
It must keep the balance and transaction history of all addresses, even the ones not designated for a wallet. In the future, we may include an API to query the address and tx history for any address. We would also easily know the richest addresses.
When a new tx arrives or a tx is updated, it must update the indexes of all affected wallets.
When the full node restarts, it should sent all notifications again. If the status of the transaction is the same, we can safely skip it.
API Service
Load from database and return to the wallet. The idea is to do the least number of calculations here. All things should be pre-computed by the Tx Processor.
Maintenance Tools
For each table, we should have a verification tool that checks its consistency. This tool should also have a parameter to recalculate the records of a given table. These tools will be integrated in our monitoring and alert system.
Database

Wallet

Id
XPubKey
AddressesMaxGap (default: 20)
Status (creating, ready, error)
CreatedAt
ReadyAt
AccessedAt (this may be useful to remove unused wallets)

Balance (only for created wallets)

WalletId (indexed)
TokenId (indexed)
Balance

Address (for all addresses, even if WalletId is null)

WalletId (indexed, null)
TokenId (indexed)
Balance (indexed)
Number of transactions
UpdatedAt

AddressTxHistory

TxId
Address
Amount (positive: received / negative: sent)
Timestamp
CreatedAt

WalletTxHistory (for all transactions)

TokenId (indexed)
WalletId (indexed)
TxId
Amount (positive: received / negative: sent)
Timestamp
CreatedAt

UTXOs

TxId
Index
TokenId
Amount
Address
TxProposalId

TxProposal

Id
Status (open, sent, cancelled)
CreatedAt
UpdatedAt
ClosedAt

Transactions (should we store it or just get from full node?)

Id (indexed)
Status (executed, voided)
Raw tx

Flow from wallets

Client creates a wallet (wallet_id = sha256d(pubkey)).
Client polls the wallet until it is ready.
Get balance and tx history for a token.
Create a tx proposal.
Sign the inputs of the tx proposal.
Resolve the tx proposal.
Propagate the tx proposal.

Events from Full Node

New tx or block.
Update tx or block.

API for the Wallet
Features:

The API can be consumed both by HTTP and Websocket.
Websocket:

Wallet must authenticate once after connection.
Wallet must received a balance update message every new tx, so it can update its balance and tx history.

Authentication
All requests must be signed by m/1/1 of the wallet's xpriv.
POST /wallet: Create a new wallet
When the wallet is created, the following tasks must be executed:

Mark its addresses in the Address table.
Calculate the balance of all its tokens and store in the Balance table.
Create the WalletTxHistory records from the AddressTxHistory.

After requesting the creation of the wallet, the client must polling to verify when it is ready. It may take a few seconds to be ready. If an error occurs, the user must have the choice to check status again or reset the wallet.
Arguments:

Pubkey
Name (optional)

Returns: { walletId, status }
GET /wallets: Get status of a wallet
Arguments:

WalletId

Returns: All fields of the wallet, including its status.
GET /addresses: Get wallet's addresses
We can run one query in the Address table filtering by a given walletId.
Open question: Should we paginate?
Returns:

List of addresses

Address
Number of transactions
Path (e.g. m/1/1, m/1/2)

GET /balance: Get wallet's balance
Run one query in the Balance table filtering by walletId (and tokenId if the argument is passed).
Arguments:

TokenId (optional)

Returns:
If TokenId is received, it will return only the requested token.

Map of balances:

Token { tokenId, tokenName, tokenSymbol }
Total
Locked
Available

GET /txhistory: Get wallet's transaction history
Arguments:

TokenId
Skip
Count

Returns:

Skip
Count
List of transactions

Format: { txId, timestamp, action, amount }
Actions: sent, received, move

GET /tx: Get transaction's detail
Arguments:

TxId

Returns:

Tx information, including decoded inputs and outputs (the same as returned by the full node)

GET /txproposals: List of wallet's tx proposals
Arguments:

Status (all, open, closed)

Returns:

List of tx proposals according to the given status

POST /txproposals: Create a new tx proposal
When a tx proposal is created, it should mark the used UTXO as locked. They do it by assigning a TxProposalId to them. So, next tx proposal will skip them avoiding double spending. We can optionally create a scheduled job to cancel all tx proposal opened for more than X minutes.
Arguments:

Input selection algorithm (optional) (e.g. best match, from highest, from lowest)
Outputs [[addr1, amount1], [addr2, amount2], ...]
Comment

Returns:

Tx proposal

Id
Comment
Raw tx (to be signed)

POST /txproposals/update: Update tx proposal
Arguments:

TxProposalId
List of input data [ inputData0, inputData1, ...] (optional)
List of parents (optional)
Nonce (optional)
Propagate bool (optional)

Returns:

Success or error message

POST /txproposal/close: Cancel tx proposal
When a tx proposal is closed, it should remove all locks from the UTXOs used by it.
Arguments:

TxProposalId

Returns:

Success or error message

One thing about the Address table and GET /addresses API.
On the table, the addresses are separated by tokenId. Imagine we had 2 transactions in total. The first just had (among the outputs) addr1 and token1. The second had addr1, token1 and addr1,token2. The table will look like this (I omitted some columns):
| Address | TokenId | # of txs | WalletId |
|---------|---------|----------|----------|
| addr1   | token1  | 2        | id1      |
| addr1   | token2  | 1        | id1      |

When we fetch the addresses for wallet with id1, how would we calculate the # of transactions for addr1? It's definitely not the sum. We could get this information from AddressTxHistory table, maybe during the API call itself (not have it pre-computed). But # of transactions on the Address table seems useless.
One other thing we might have is both an Address and AddressBalance tables.
AddressBalance

Address
TokenId
Balance

Address

Address
Path
WalletId
# of txs

I'll go with the suggestion I gave on the last comment for addresses.
We need to store # of txs for each address in order to know if it's been used or not. We cannot use just the balance, as the address might've been used but happens to have balance 0.

# of txs is also used on the explorer, when we show the # of txs for the address for each token.
One thing I noticed from the design is that it's missing an address on the Address table. Probably you've already added it.
And I agree the best approach would be to get the tx count from the AddressTxHistory table during the API call.

And I agree the best approach would be to get the tx count from the AddressTxHistory table during the API call.

I'll actually do as I suggested, so there should be a field on the database for the # of txs and we won't need to calculate it.
The reason for this is that we need a way to know if an address has been used or not. Just having the balance doesn't help, as the address might have been used but happens to have balance 0.

The reason for this is that we need a way to know if an address has been used or not. Just having the balance doesn't help, as the address might have been used but happens to have balance 0.

This could be easily done checking if there is any data on AddressTxHistory for this address.
But it's okay, I am good to go with your solution. You just need to add # of txs field also on the AddressBalance table for the reason I mentioned.

This could be easily done checking if there is any data on AddressTxHistory for this address.

That's true. But I think I'll move with 2 separate tables to have it pre-computed (and can also store the address derivation path on the Address table).

You just need to add # of txs field also on the AddressBalance table for the reason I mentioned if you go with it.

Makes sense, I hadn't thought of that. Thanks.

Ok, so for the locked/unlocked balances, this is what I thought.
The AddressBalance table would require locked/unlocked balances:
| address_balance   |
|-------------------|
| address           |
| token_id          |
| locked_balance    |
| unlocked_balance  |
| n_of_transactions |

The same for WalletBalance:
| wallet_balance    |
|-------------------|
| wallet_id         |
| token_id          |
| locked_balance    |
| unlocked_balance  |
| n_of_transactions |

The Utxo table already has a timelock column:
| utxo     |
|----------|
| tx_id    |
| index    |
| token_id |
| value    |
| timelock |

I see 2 possibilities here. We can:

use this timelock column to indicate if it's locked. If the timelock is already past the present time, the column will be set to null. We don't preserve the information, but after the tokens are unlocked I'm not sure it's worth having it;
add an extra locked column that will indicate whether the utxo is locked or not. So if an utxo has a timelock in the past, the timelock column will still be set but the locked column will be null (or false).

I believe there are 2 "external" factors we should also consider: block height lock and tx-proposals. Both lock a tx, but not with a specific timestamp. Using idea 1, we could set the timelock column to MAX_INT when a block is not yet unlocked or an utxo is part of tx-proposal. This way they'll be considered locked. Later, when they are unlocked, it should be set to null.
To be honest, I haven't seen any advantages or disadvantages in either. Re-using the timelock column should do the trick, but adding an extra column preserves the utxo timelock information (not useful now, but might be useful in the future, even though it's easy to get it directly from the tx as we have the tx_id and index).
What do you think?

One other possibility is doing like proposal 1 above, but never deleting the timelock info from the table. When we want to create a tx proposal, we query the table and filter by timelock <= some_date. This way we get only those that are unlocked.
For blocks, since they are unlocked by height, we could set the timelock to MAX_INT and later remove it.

I wonder how useful is the timelock information, I'd guess it is somewhat useful so I'd say proposal 1 with timelock <= some_date on queries seems the best of both worlds (although performance-wise, it really isn't obvious which is best, and I think this is fine). As for blocks, couldn't we just add a heightlock and use heightlock <= some_height on queries? These even work well with NOT_NULL because a value of 0 can be used on both cases to indicate no lock. I don't really see the problem of adding a column.

As for blocks, couldn't we just add a heightlock and use heightlock <= some_height on queries? These even work well with NOT_NULL because a value of 0 can be used on both cases to indicate no lock. I don't really see the problem of adding a column.

I like this idea. We'd also always keep the information in the tables (no need to set to null), which may be even better when dealing with conflicts, as the best block height might change. I'll follow it.

Did some research on how we could deal with timelocks. It's a bit more complex than heightlock. The latter is easy to handle because a new block unlocks every time a new one arrives. We don't need to schedule anything.
For timelocks, these are some suggestions:

Check for new unlocked transactions every time a block arrives. It should be around 30s interval, so seems like a precision we can tolerate, even though it can take up to a few minutes. This the simplest solution;
We can schedule events. They enable us to run lambda functions at a set date/time. It's a pretty straight-forward. However, we can only schedule events with minute precision, so doesn't seem to have much advantage over the first solution;
AWS SQS allows us to set a timer for the message, so it's only delivered after that time. We can have an unlock message sent to the queue with the timer expiring at the exact moment the utxo should be unlocked (I haven't been able to find if this timer is 100% precise or a best effort). It's not very simple because the maximum delay time is 15 minutes. We'd have to run a lambda every 15 minutes (using schedule events) that sends messages with all utxos being unlocked in the next 15 minutes, using the correct delay. There is also a corner case where a new tx arrives with a timelock due to less than 15 min and we need to handle, but the basic idea is the same. This is the most complex idea, but the one that guarantees txs are unlocked precisely.
We could also do a combination of 2 and 3. Instead of running a lambda every 15 minutes, schedule it when we get an utxo with timelock (we should round to the minute) and then send the delayed message. So if we get an utxo that unlocks at 01:35:40pm, we schedule a function to run at 01:35pm and it sends a message with 40s delay, which should run at the precise unlock time. Also a bit complex.

What do you think? I'm inclined to go with 1. Seems like a reasonable precision.

The problem is not entirely clear to me yet. Let me see if I understand.
These affects at most 3 tables address_balance, wallet_balance and utxo, so I'll only focus on them:

It seems like utxo can be simplified a lot by using timelock and heightlock that way row operations should basically be insert and delete, and those are made only made for every block update, which seems great.
Now address_balance and wallet_balance have the columns locked_balance and unlocked_balance which potentially depend on both the current time and the current block height. Which is what we're trying to deal with.

Ideally locked_balance and unlocked_balance would behave "calculated fields" (like properties in python), but that is inefficient, that's probably the reason these fields exist in the first place. So they are basically a cache.
All the proposals 1 to 4, focus on how to update every one of these cached values as reasonably soon as possible. But I don't know if this is necessarily great. I'll add a proposal 5. A "lazy evaluation", add locks_expire_height and locks_expire_timestamp columns on both address_balance and wallet_balance, and when only when querying wallet_balance, if the balance is invalid (because then querying we have the latest timestamp and height) we update the locked/unlocked balances of the wallet and its addresses. I don't imagine this being more complex than configuring a trigger system to update all balances, but please do correct me on this one.
If we have to go with one of these proposals, I'd go with 1. because it doesn't seem to need any extra configuration, we already have a stream of block events. But the trade offs I see are: a) having a slightly outdated balance lock (~30s) vs having to wait for a balance update query (probably a fast query); b) one query to update all balances, even when they are not being used vs multiple queries that only update the balances there are being requested.

It all depends on how much processing we wan't to do when there's an api call. Let's use your proposal and imagine someone queries the wallet balance, which has a locks_expire_timestamp set. From just that information, we don't know which utxo is now unlocked. We'd have to find out which addresses belong to this wallet and then query the utxo table to find out the unlocked utxo(s) (or just a sql join). Anyway, seems like the kind of thing we were trying to avoid by calculating everything beforehand.
@jansegre not sure I got your idea right or I'm not seeing an easier way to implement it.

Wait, shouldn't UTXOs have an address field? It would be possible if tx_id and index was enough to find the address, but I think it would be useful to have the address on the utxo table. I know it's one more field, but this seems like a useful one, no? The rest I assume is there already (there should be a way to find all UTXOs that make up a wallet through tx_id and a transaction-wallet table (which I imagine we have, because we have to list all transactions of a wallet). Or if we add a utxo_wallet or addrerss_wallet index pair table.

We do have the address on the utxo table. As I said, we'd have to query:

We'd have to find out which addresses belong to this wallet and then query the utxo table to find out the unlocked utxo(s) (or just a sql join).

But I think this design tries to prevent as much as possible these on-demand calculations and prefers to have pre-computed values.
Also, we might have more than one utxo with timelock. In that case maybe we'd need a list of locks_expire_timestamp. To be honest, I'm still leaning a bit more towards the other solutions.

I think @jansegre's idea seems better, it doesn't add much more processing (even though it adds more complexity to the code). Updating the  address_balance is easy and if we add a wallet-id column to the utxo table it's also easy to update wallet_balance balance.
The good thing is that we will always have the correct balance, even if there is a bug in our mining and we end up having some hours without blocks.
The bad thing is that we need to do this check (see if there ia a balance to be updated) every time someone requests the balance (on the wallet, or on the explorer to get an address balance, or the richest addresses). However this check would be needed every time a block arrives in the first option, so I don't see that as something really bad.

We do have the address on the utxo table. As I said, we'd have to query:

We'd have to find out which addresses belong to this wallet and then query the utxo table to find out the unlocked utxo(s) (or just a sql join).

I just read DATABASE.md and it looks like we do have everything we need. I misunderstood that we needed more info to make this query possible. And it seems to me that the whole update could be made with one or two queries, which admittedly aren't trivial, but probably won't have more than 2 joins each, and would have to visit all utxos of the addresses needing an update. Which even in the range of 10k seem to me like it should be a nearly instant query (at least with the right indexes) for MySQL.
Then again, similarly could be said for a query that updates all expired locks globally, but that would scale much worse, for the benefit of always having a balance at hand that will be outdated sometimes. If we accept that the balance can be outdated sometimes, we could return the current expired balance before waiting for the update query to end.

But I think this design tries to prevent as much as possible these on-demand calculations and prefers to have pre-computed values.

I agree with the sentiment, but I don't fully agree "as much as possible", there are other trade-offs to consider. This query would most likely have to be run the first time a miner (assuming a miner because it's the most common wallet user with locks) opens their wallet and then every N seconds, N being how long it takes for them to find a block. And since I'm assuming this query won't take more than a few hundred milliseconds (if even that), it's a very fair price to pay for scaling better, always returning the correct up to date balance, and having a slightly less complicated deploy.
An orthogonal idea (that can probably be used along whatever we decide to do with locks and leans more towards pre-computed values) would be pre-computing a list of future "(unlocked, locked)" balances, that approximates (because it would have to somewhat synchronize heightlock+timelock) when the next unlocks will be and pre-computes the next N (for some N, or even until there aren't any locks, which shouldn't be a too high of a count) balances (and in the background update the forecasts and delete the past balances). When we receive a query we just use the balance that is valid at the timestamp the query was made. This could be a relevant if whatever solution we go for ends up having a bad performance.

Also, we might have more than one utxo with timelock. In that case maybe we'd need a list of locks_expire_timestamp.

That's what I imagined, and I think the same query would update all the balances needed. What I keep thinking is "how hard can it be to make this query?". I may very well be underestimating this, but if it's mostly matter of "this will make the wallet slow", I think we could make this query and benchmark it a little (or against updating all addresses, considering the UTXOs already on the mainnet) to have an idea of the performance impacts.

Just 2 things about this idea:

I'm not sure it's worth doing this for heightlock. We can already update it when a block arrives, because that's when the old block becomes unlocked. It's a pretty simple operation, so I don't feel there's any advantage in doing it differently. For timelock it's a different thing;

if we decide to go this route, it's important to also discuss the sync strategy between wallets and this service. For instance, the current wallet only queries information from the full node when it starts and then listen for the transactions on websocket. In this case, there would be no new queries to trigger this balance update. So if I open a wallet one minute before a timelock expires, I wouldn't make another query later and get the new unlocked balance;

2. if we decide to go this route, it's important to also discuss the sync strategy between wallets and this service. For instance, the current wallet only queries information from the full node when it starts and then listen for the transactions on websocket. In this case, there would be no new queries to trigger this balance update. So if I open a wallet one minute before a timelock expires, I wouldn't make another query later and get the new unlocked balance;

To me this is actually the best argument against it so far, and probably tips the balance in favor of your initial proposals (of some form of event that would trigger the update).
The only alternative I see that could possibly solve this without relying on time based events, is to move away from a simple (locked, unlocked) balance structure to something like [(0, <full balance>), (-1, <balance locked from height or "permanently locked"> (<ts1>, <balance unlocked after ts1>),(<ts2>, <balance unlocked after ts2>), ...], which the UI can use for exposing a (unlocked, locked) balance or even display  when the next amount would be unlocked, but this is obviously more complicated and I'm not sure I like it that much.
I think we could review the balance structure when we include support for multisig. I suspect some sort of improvement of this structure might be inevitable. Also we should consider researching how BWS does this (and possibly other wallets too), it's probably a problem that has been solved many times in many different ways.
For now, I'm leaning into the proposal 1., piggy-backing on block events to get these "time-events" that would signal a possible unlock event.

I think that @jansegre's idea is better and simpler. To reduce some of the downsides pointed out by @obiyankenobi, we can run once a "general utxo update" task once a day (twice a day?) that will go through all utxos that have timelock <= now (we need to properly index the db) and update their wallets.
Regarding the sync between wallet and the service, I don't like @jansegre's idea of storing multiples balances according to the timestamp. We can simply include a next_refresh field that will be equal to the minimum timelock of the utxos of that wallet (or null). So, even if no transaction has been received for a while, the wallet will trigger the update. What do you think?

We can simply include a next_refresh field that will be equal to the minimum timelock of the utxos of that wallet (or null). So, even if no transaction has been received for a while, the wallet will trigger the update.

We'd have 1 next_refresh value per wallet? How does the wallet get this value, in case it's been changed? We again fall on the case of having to debate the sync between wallet and this service, as this value needs to be updated.

We'd have 1 next_refresh value per wallet? How does the wallet get this value, in case it's been changed? We again fall on the case of having to debate the sync between wallet and this service, as this value needs to be updated.

I think an easier way to see this is looking at the balance returned by the API as the following structure (which doesn't have to be the structure we use, but it would be functionally identical apart from a refactor):
balance: {
  available: X,
  locked: Y,
  expires: T,
}

The wallet is then able to display the balance until the time reaches expires, when it will have to make a "refresh" query to get the updated balance (including the new expires).
This value represents the earliest timelock to expire, and it can't change unless a new tx/block for this wallet arrives, and then this is an event that should trigger the same update mechanism that a schedule would (which I assume is a solved problem, how to make this update arrive to the frontend).
The more I think about it, the more I get that this is just the functionality relevant <0.1% of uses that requires >30% of the design effort. By no means I think we should ignore this, but we definitely can postpone this and let time-locked balances be outdated sometimes, no?

I agree with @jansegre that it's okay to let the balance be outdated sometimes. I just think we shouldn't prevent the users from use the tokens only because we haven't updated it.
That's the reason I think we could go with the first solution proposed with the timelock updates when a new block arrives. My only concern is if we have a long time without blocks, we would have an outdated balance for a long time but this not exactly  a big problem, since timelock is not used every time. So to allow the use of tokens even if we haven't received any blocks, I think we could update the wallet timelocks every time we request a tx proposal also, so the users would never have any problems when trying to spend an output that is not locked anymore. This is not ideal because it will show one thing on the balance and another thing is for real but I guess it's fine.

and then this is an event that should trigger the same update mechanism that a schedule would (which I assume is a solved problem, how to make this update arrive to the frontend).

That's true, hadn't thought about it this way.
I agree that it's ok to have the balance outdated for a little while. @pedroferreira1 I'm not sure that we necessarily need to handle this, as the user will see the balance as locked and the wallet currently will show an error if he tries to send more tokens than he has. So on the user's point of view, it'll be consistent.
But I'm also thinking more about this lazy evaluation proposal and it might not be much more complex to implement. Also, as @jansegre suggested, we should look at how BWS handles this.

From my research, it seems BWS does not handle height or time locks. From bitcore-wallet-service/src/lib/chain/btc/index.ts:
  totalizeUtxos(utxos) {
    const balance = {
      totalAmount: _.sumBy(utxos, 'satoshis'),
      lockedAmount: _.sumBy(_.filter(utxos, 'locked'), 'satoshis'),
      totalConfirmedAmount: _.sumBy(_.filter(utxos, 'confirmations'), 'satoshis'),
      lockedConfirmedAmount: _.sumBy(_.filter(_.filter(utxos, 'locked'), 'confirmations'), 'satoshis'),
      availableAmount: undefined,
      availableConfirmedAmount: undefined
    };
    balance.availableAmount = balance.totalAmount - balance.lockedAmount;
    balance.availableConfirmedAmount = balance.totalConfirmedAmount - balance.lockedConfirmedAmount;

    return balance;
  }

In this context, locked means transactions which are part of a tx proposal and confirmed means transactions which have at least one new block confirming them.
This print in one of their issues shows the explanation in the Copay app.

To me this also raises the concern of confirmations. We aren't really doing anything, and I think it's more important time locks.

To me this also raises the concern of confirmations. We aren't really doing anything, and I think it's more important time locks.

I agree that it's an important topic to be discussed, maybe on a separate issue.

I started to think about tx proposals. Some stuff is not clear to me:

I think the tx-proposal idea of BWS is to handle multisig. Is this also our goal? If it is, we need to handle “collecting” signatures from multiple users and storing that while the tx is only partially signed, before sending. There are no tables for this in the design;
we could also accept a list of inputs on POST /txproposals, so the user can choose the inputs. It’s probably easier than creating the proposal and later updating with the inputs (with POST /txproposals/update);
what’s the advantage of the wallet-service sending the tx? I’d say it’s better that it only deals with assembling the tx. Then the wallet can do the pow and propagate the tx;
do we need to store old proposals? I think we can have on database only the open ones. In that case, not sure if it’s worth having a status, as they’re all open.
also, if we want to be able to update the proposal, we need to store the original information, such as the outputs. Currently there’s no place for that;

I'm starting to wonder if it's not better to have a super simple flow. It'd be like this:

wallet calls POST /txproposal with the outputs (or just the total amount needed);
wallet-service selects the UTXOs, marks them as part of a proposal and returns them to the wallet;
wallet assembles the final tx, does pow (with mining service) and propagates tx. If send is not successful or user cancels, tell wallet-service to close the proposal;

1. I think the tx-proposal idea of BWS is to handle multisig. Is this also our goal? If it is, we need to handle “collecting” signatures from multiple users and storing that while the tx is only partially signed, before sending. There are no tables for this in the design;

I'd say we don't need to support multisig right now. But ideally we make an API that is identical, or very close, or at the very least compatible, with the API that supports multisig. Unless supporting multisig is easy (like an extra day of work maybe), than I'd be in favor of supporting it from the get go.

2. we could also accept a list of inputs on `POST /txproposals`, so the user can choose the inputs. It’s probably easier than creating the proposal and later updating with the inputs (with `POST /txproposals/update`);

As long as the API can work without having to take a list of inputs. If the API requires explicit inputs, it won't improve the current situation of the wallet, and any updates on that logic will take much longer because they need to be deployed to the end-users. But manually choosing the inputs is something the current wallet can already do, it would be bad to take this feature away, even if only temporarily.

3. what’s the advantage of the wallet-service sending the tx? I’d say it’s better that it only deals with assembling the tx. Then the wallet can do the pow and propagate the tx;

The biggest advantage is not exposing any API of the fullnode to the wallet. And this is no small advantage. I'm not saying the proposal API has to take care of propagating, maybe it does.

4. do we need to store old proposals? I think we can have on database only the open ones. In that case, not sure if it’s worth having a status, as they’re all open.

It probably makes sense for a couple of reasons. We can offload old proposals to a log db to help us identify issues (privacy wise it's debatable, but maybe all or most of the info stored on the proposal would end up anyway on the blockchain if it was successfully propagated) and the process would collect "done" or "cancelled" proposals and then delete them.

5. also, if we want to be able to update the proposal, we need to store the original information, such as the outputs. Currently there’s no place for that;

I'd say we can start with immutable proposals. There may be pitfalls in supporting updates, it might make sense for multisig, but don't know.

I'm starting to wonder if it's not better to have a super simple flow. It'd be like this:
1. wallet calls POST /txproposal with the outputs (or just the total amount needed);

2. wallet-service selects the UTXOs, marks them as part of a proposal and returns them to the wallet;

3. wallet assembles the final tx, does pow (with mining service) and propagates tx. If send is not successful or user cancels, tell wallet-service to close the proposal;

I'd prefer if the wallet-service was responsible for propagation. The only difference would be that after the tx is assembled the wallet-service marks it as needing POW (which would use the state or status field), the wallet chooses how to handle POW on its own, and when it has the final info (nonce and maybe timestamp), it asks the wallet-service to propagate the tx. The advantage of that is that the wallet service can keep locks for UTXOs with more confidence, whether if the tx was propagated outside it's knowledge it would have to wait for the node to detect that an UTXO was spent, or timeout the UTXO lock.

I'd say we don't need to support multisig right now. But ideally we make an API that is identical, or very close, or at the very least compatible, with the API that supports multisig.

To be honest, I'm not sure how much extra time that would be. But I think we didn't design any other part of the service thinking about multisig wallets. If we want to support it, we'll have to change all of them also, so I don't think it's much use designing this part thinking about multisig if the rest doesn't support it.

As long as the API can work without having to take a list of inputs.

Of course, the inputs would be optional.

The biggest advantage is not exposing any API of the fullnode to the wallet.

That makes sense, but I worry if that would create a dependency in other services. We'll start propagating txs using the tx-mining-service, so the wallet-service would have to communicate with that one.

It probably makes sense for a couple of reasons.

It's still not clear to me the benefits of keeping old proposals. If it's just for debugging, I'd say just logging can take care of that.

The advantage of that is that the wallet service can keep locks for UTXOs with more confidence, whether if the tx was propagated outside it's knowledge it would have to wait for the node to detect that an UTXO was spent, or timeout the UTXO lock.

To be honest, even if we propagate from the wallet-service, I'd say we should wait for the confirmation from the full node. Since we'll "lock" UTXOs that are part of a transaction, I'd say it's not a problem to wait for this confirmation, there's no risk of trying to use the same UTXOs twice. And I think we'll have to have a "cleanup" process anyway.

I think we should discuss multisig later, no need to add support right now and we can discuss in a later design;

I agree that we should add support to receive the inputs on /txproposals POST and not updating it. The input selection might change also the outputs (with a change output), so I would say we support it only in the creation. Also I think would be good to prepare this part of the code to accept the an input selection algorithm later (I am not saying to add support now, only to code this part thinking about it).

I agree with Jan that this could be done by the wallet-service. At first I thought it would be better to leave it to the wallet because we could decide to resolve the pow of the transaction on the client later (or have another third party paid service that resolves tx pow, as we discussed sometimes). But with the reasoning of not exposing the API I don't see any problems in returning to the wallet the 'unresolved tx', resolve the pow and then send to the wallet service to propagate it.

That makes sense, but I worry if that would create a dependency in other services. We'll start propagating txs using the tx-mining-service, so the wallet-service would have to communicate with that one.

The tx-mining-service only solves the pow of the transaction, it's not propagating anything (it could but we decided not to), so nowadays the wallet is propagating the transaction even after the tx mining integration.

I also don't think we should save old proposals but the status is important to know if it's creating the proposal, waiting for pow, waiting for propagation, cancelled, or completed. Thinking about this 'cancelled' status maybe it's good to have the proposals on the database (for a while at least) to get this stats.

Expanding a bit on this point with a practical example, multisig wallets don't have xpubkeys like regular wallets. The service so far was designed relying on having the xpubkeys, so adapting to multisig will require a major overhaul.

Makes sense. I suppose it isn't of much practical use to support multisig transactions but not wallets, since it could be hard to use the utxo later. Unless this useful for the headless-wallet, we could skip considering multisig for now.

That makes sense, but I worry if that would create a dependency in other services. We'll start propagating txs using the tx-mining-service, so the wallet-service would have to communicate with that one.

The tx-mining-service only solves the pow of the transaction, it's not propagating anything (it could but we decided not to), so nowadays the wallet is propagating the transaction even after the tx mining integration.

Yeah. I agree that creating inter-service dependency is not good. And I think we can still do this without it. The wallet-service should not communicate with tx-mining-service whatsoever. The wallet is the one that communicates with both. And both communicate with a fullnode (or multiple fullnodes, it's up to each service).

I think the tx-proposal idea of BWS is to handle multisig. Is this also our goal? If it is, we need to handle “collecting” signatures from multiple users and storing that while the tx is only partially signed, before sending. There are no tables for this in the design;

I agree that we can design multisig later. I don't think adapting the service will require a major overhaul, but we can dive deep into it in another design.

we could also accept a list of inputs on POST /txproposals, so the user can choose the inputs. It’s probably easier than creating the proposal and later updating with the inputs (with POST /txproposals/update);

I agree that we can accept a list of inputs on POST /txproposals. I think the POST /txproposals/update is useful in other cases, such as Atomic Swap. When its just user A sending funds to user B, it doesn't seem relevant.

what’s the advantage of the wallet-service sending the tx? I’d say it’s better that it only deals with assembling the tx. Then the wallet can do the pow and propagate the tx;

I also prefer to publicly expose only the wallet service API. The wallet wouldn't need to have access to the full node at all.

do we need to store old proposals? I think we can have on database only the open ones. In that case, not sure if it’s worth having a status, as they’re all open.

The status may be important in other cases. For example, in a multisig transaction, the status may let the other parties that a transaction has been cancelled. Or that one of the signers declined to do it. In our case here, the status would indicate whether the transaction was successfully created and is just waiting for the correct nonce and parents.

also, if we want to be able to update the proposal, we need to store the original information, such as the outputs. Currently there’s no place for that;

I didn't understand why we need to store the original information.

I'm starting to wonder if it's not better to have a super simple flow. It'd be like this:

wallet calls POST /txproposal with the outputs (or just the total amount needed);
wallet-service selects the UTXOs, marks them as part of a proposal and returns them to the wallet;
wallet assembles the final tx, does pow (with mining service) and propagates tx. If send is not successful or user cancels, tell wallet-service to close the proposal;

I can't see the difference between this super simple flow to the one in the design.

Yeah. I agree that creating inter-service dependency is not good. And I think we can still do this without it. The wallet-service should not communicate with tx-mining-service whatsoever. The wallet is the one that communicates with both. And both communicate with a fullnode (or multiple fullnodes, it's up to each service).

@jansegre Another advantage of not propagating the transaction using the tx-mining-service is that to avoid the following case: (i) wallet sends the tx to the tx-mining-service, (ii) wallet's connection is down, so an error message shows up, and (iii) the tx-mining-service propagate the transaction. In this case, the user would see an error message while the tx-mining-service would have sent the transaction.
I know that this case may happen in the propagate request, but this is a very fast request. The request to the tx-mining-service is way slower and requires polling to check if the tx has already been resolved.

To be honest, even if we propagate from the wallet-service, I'd say we should wait for the confirmation from the full node. Since we'll "lock" UTXOs that are part of a transaction, I'd say it's not a problem to wait for this confirmation, there's no risk of trying to use the same UTXOs twice. And I think we'll have to have a "cleanup" process anyway.

I agree. We should mark a transaction as sent (and release its "UTXOs") only after the full node has responded that it was successfully propagated. In this case, we should also wait for the notification from the full node before we release the UTXOs. Otherwise we may be subject to a race condition.

Ok, from the comments we seem to have agreed on skipping any multisig design for now and that this service will be responsible for propagating the tx to the full-node. The flow I propose is this:

wallet sends POST /txproposals to create a tx. Parameters:

outputs (we might complete it with the change output);
(optional) input selection algo;
(optional) inputs;

wallet-service gets the utxos corresponding utxos and also creates a tx proposal in the DB, with a open status. Response:

inputs;
outputs;

wallet does the PoW with the tx-mining-service;

wallet uses POST /txproposals/update to send the PoW, parents and timestamp, also asking to propagate the tx. Parameters:

nonce;
parents;
outputs;
inputs;

wallet-service assembles the final tx and propagates it;

On step 4, we need to receive both the outputs and inputs:

inputs: the inputs are actually utxos that will be spent and we already have this info in the database. We can just add a tx-proposal column to the utxo table, so in theory we don't need to receive them again. However, the order of the inputs is important to assemble the final tx. If we just have the tx-proposal id in the utxo table, we can't guarantee we'll assemble the final tx with inputs in the same order;
outputs: these are new outputs, utxos that will be created, so we don't have this info in the database yet. We need to receive them, unless we also store them when we first create the proposal;

In the end, it might be easier to just receive the complete tx in step 4, instead of each piece. The wallet can assemble it and send the hex. The wallet-service wouldn't need to assemble it, just propagate.

2. wallet-service gets the utxos corresponding utxos and also creates a tx proposal in the DB, with a `open` status. Response:

* inputs;

* outputs;

It might be worth saving these input and outputs to the database. For the inputs, we can link them to the utxos which would help preventing selecting an utxo that is already in use by another open proposal, and also saving the order of that input. Outputs benefit from having the order stored and also being a cache so it doesn't need to be passed back by the wallet after the tx is mined.
I'm also not sure what is enough for the tx-mining-service to mine a transaction. Does it need a timestamp hint, tx version, parents, or anything else? If it does this would be the call that needs to return those params.

4. wallet uses `POST /txproposals/update` to send the PoW, parents and timestamp, also asking to propagate the tx. Parameters:

Not sure what other endpoints do but this could be a PUT /txproposals. No substantial benefit to this except being more RESTful (but not completely), but also no downsides either. If other endpoints follow these POST /x/update scheme, then I wouldn't bother with this alternative.
Otherwise this proposal seems great.

Not sure what other endpoints do but this could be a PUT /txproposals. No substantial benefit to this except being more RESTful (but not completely), but also no downsides either. If other endpoints follow these POST /x/update scheme, then I wouldn't bother with this alternative.

I agree. Also, I am wondering if we should use path parameters instead of query ones:
POST /wallets/       --> creates the wallet
GET /wallets/{walletId}
GET /balances/{walletId}
etc...

POST /txproposals/     --> creates the tx proposal
PUT /txproposals/{txProposalId}

The only reason I am not 100% convinced is that no other service of ours uses this standard, if I'm not mistaken. For example, fetching a tx in the full node uses GET /transactions/?id=tx_id.

In the end, it might be easier to just receive the complete tx in step 4, instead of each piece. The wallet can assemble it and send the hex. The wallet-service wouldn't need to assemble it, just propagate.

I do agree with this, it's probably easier to receive a hex tx and just propagate it to the full node. However I think we should have a validation here, that the inputs/outputs from the hex data are the same as the ones in the proposal. Otherwise we might be locking some outputs that were not used.

I'm also not sure what is enough for the tx-mining-service to mine a transaction. Does it need a timestamp hint, tx version, parents, or anything else? If it does this would be the call that needs to return those params.

tx-mining-service needs tx data without nonce and parents. It returns the parents, nonce and timestamp.

I do agree with this, it's probably easier to receive a hex tx and just propagate it to the full node. However I think we should have a validation here, that the inputs/outputs from the hex data are the same as the ones in the proposal. Otherwise we might be locking some outputs that were not used.

This will only happen if the wallet modifies the tx, which is not expected. Anyway, when the tx is sent (and confirmed by the full node), we can remove all proposal ids from the utxos, if there are any left. No utxos would be left locked, so there's no need to double-check this information.

Outputs benefit from having the order stored and also being a cache so it doesn't need to be passed back by the wallet after the tx is mined.

It's still not clear to me what's the advantage of saving them. In most cases, we have only a few outputs, so that would not make a big difference regarding bandwidth.
One other thing I forgot on step 4 is receiving the signature for each input. I think this also supports the idea of sending the full tx.
On the other hand, assembling the tx on the wallet-service might serve us if we decide to change its structure in the future. Instead of everyone having to update the wallet, we just need to update the wallet-service.

I agree with all points you made. Maybe we should put this design in another Design Issue and put the link in the description of this one. Or should we update this one?

Which design? For tx-proposals?

Points to decide:

are we gonna save the inputs and outputs of a proposal on the wallet-service?
will we receive the complete tx or just the weight, nonce, etc on the tx-proposal update API?

Points to decide:

are we gonna save the inputs and outputs of a proposal on the wallet-service database?

I think we should mark the selected inputs as "used" in the UTXO table. It seems similar to saving the inputs, except for the input data, right?
Regarding the outputs, I think we must save them as well to know where the funds will be transferred to, and also which address was used for the change. Maybe I'm not seeing the whole picture.
I can't see how we can move forward without saving the inputs and outputs. We can talk tomorrow about it.

will we receive the complete tx or just the weight, nonce, etc on the tx-proposal update API?

I think the tx-proposal update API can have a bunch of optional fields. It will update only the received fields. It can accept weight, nonce, parents, inputs, input[0], outputs, output[0], and so on. If it gets too complicated, we can do the bare minimum to get the service working and extend it later.

I think we should mark the selected inputs as "used" in the UTXO table. It seems similar to saving the inputs, except for the input data, right?

Almost the same. But we'll have not only to mark the UTXO as used, but also its position in the input list of the new tx.

Regarding the outputs, I think we must save them as well to know where the funds will be transferred to, and also which address was used for the change. Maybe I'm not seeing the whole picture.
I can't see how we can move forward without saving the inputs and outputs. We can talk tomorrow about it.

We can if we require the user/wallet to send us the complete tx after it does the pow. The wallet will have all the information it needs to assemble the tx.

I think we should mark the selected inputs as "used" in the UTXO table. It seems similar to saving the inputs, except for the input data, right?

Almost the same. But we'll have not only to mark the UTXO as used, but also its position in the input list of the new tx.

We could just have a table for inputs and outputs of open transaction proposals (which would include a column to indicate the position of the output/input on their respective tables). I think that's what I suggested, this should be fine, right?

Regarding the outputs, I think we must save them as well to know where the funds will be transferred to, and also which address was used for the change. Maybe I'm not seeing the whole picture.
I can't see how we can move forward without saving the inputs and outputs. We can talk tomorrow about it.

We can if we require the user/wallet to send us the complete tx after it does the pow. The wallet will have all the information it needs to assemble the tx.

I think that accepting a whole tx for the sake of not having to assemble it is a bad idea. But accepting a whole tx for the sake of convenience (if that's what the tx-mining-service returns), is OK. We should at the very least verify if the transaction we're sending is of an open tx proposal, which would require breaking up an "assembled tx". Mostly to mitigate propagating transactions unrelated to the current wallets ("rogue transactions").
On the other hand, if the wallet service only receives the missing parts of the tx (instead of an "assembled tx"), there is no way to even propagate a rouge transactions, because there wouldn't even be a way to build one.

We should at the very least verify if the transaction we're sending is of an open tx proposal, which would require breaking up an "assembled tx". Mostly to mitigate propagating transactions unrelated to the current wallets ("rogue transactions").

The user can indicate the proposal-id when sending the tx, so we don't need to "break it up" to check. In the end, we'll only update most of the database tables when we receive the transaction from the full node, when it's been propagated and it's valid.
We'll need a "cleaner process" anyway, to clean up utxos that are marked as part of a tx proposal but were eventually not sent. That would also serve in case the user sends a tx with incorrect proposal id. Let's see how that would work:

User creates the tx proposal. The wallet service selects utxo0 and utxo1 as inputs and mark with txProposalA;
User decides to use a different input (utxo2), does the pow and asks to propagate the tx (tx1);
Wallet service receives tx1 from the user and propagates it. At this point, it does not remove the proposal id from the utxo table;
Wallet service receives tx1 from the full node. At this point, it'll remove the proposal id from the utxo table. In fact, it removes the utxo from the table, as it's been spent. utxo0 and utxo1 are left there marked as being part of txProposalA;
After some time, the cleaner process will check all utxos that are marked as part of a proposal and see which ones should be unmarked, based on how "old" the proposal is. At this point, we'd remove txProposalA from utxo0 and utxo1;

Only one problem can happen as result of this mismatch: utxo2 is not marked as part of proposal and it will be available to be chosen as an input until we get tx1 from the full node. If the wallet tries sending another tx immediately after sending tx1, it might use utxo2 and it'll get an error when propagating, as utxo2 has been spent by now (we have this problem currently in our wallets). Given that this will only happen if the wallet misbehaves, I think it's acceptable.

It still bothers me that the wallet service would be able to propagate any transaction where there is an open proposal-id, even if this transaction is totally unrelated to the transaction being proposed. That would make that API almost like a proxy to push-tx. Is there any other upside besides simplifying implementation a little? Even if we have to implement a little more to assemble txs (which really shouldn't be much), it could help catch bugs on the client end that could otherwise be completely silent.
I agree that we'll need a cleaner process for the sake of catching up what eventually passes through the cracks. But allowing the tx-proposal to transparently push transactions to the fullnode is relying too much on the cleaner process. Ideally the cleaner process would log an error or warn us every time it had to actually cleanup something, and that would indicate a bug.
That's not to say that accepting an encoded transaction is a bad format, maybe it's still more convenient, but I do think that some validation that would require inspecting the encoded transaction is needed.

It still bothers me that the wallet service would be able to propagate any transaction where there is an open proposal-id, even if this transaction is totally unrelated to the transaction being proposed. That would make that API almost like a proxy to push-tx. Is there any other upside besides simplifying implementation a little?

This only happens if the wallet misbehaves, so I don't see as a big problem.

Even if we have to implement a little more to assemble txs (which really shouldn't be much), it could help catch bugs on the client end that could otherwise be completely silent.

I'm not sure what you mean by that. Do you mean if the wallet assembles the tx incorrectly? The wallet service will only work in conjunction with the wallets, anyway. They'll have to send the correct data in the API, so whether it's the list of outputs or the complete tx.

Is there any other upside besides simplifying implementation a little?

I think simplifying the code seems like a very good reason, since it's not clear the benefits of the more complex solution. The benefit I see in assembling it on the wallet service is that, if we make changes to the tx structure, we only need to change the wallet service, not require all users to update the wallet.

It still bothers me that the wallet service would be able to propagate any transaction where there is an open proposal-id, even if this transaction is totally unrelated to the transaction being proposed. That would make that API almost like a proxy to push-tx. Is there any other upside besides simplifying implementation a little?

This only happens if the wallet misbehaves, so I don't see as a big problem.

But that's exactly it. Misbehaving means bugs, and we're making that bug harder to find.

Even if we have to implement a little more to assemble txs (which really shouldn't be much), it could help catch bugs on the client end that could otherwise be completely silent.

I'm not sure what you mean by that. Do you mean if the wallet assembles the tx incorrectly? The wallet service will only work in conjunction with the wallets, anyway. They'll have to send the correct data in the API, so whether it's the list of outputs or the complete tx.

The wallet client or the tx-mining-service could have a bug, or there could be some exploit possible to sending a custom transaction through the wallet service, which could be cheaper than setting up a fullnode (but I'm not really sure on this).

Is there any other upside besides simplifying implementation a little?

I think simplifying the code seems like a very good reason, since it's not clear the benefits of the more complex solution. The benefit I see in assembling it on the wallet service is that, if we make changes to the tx structure, we only need to change the wallet service, not require all users to update the wallet.

I think my bias is that I'm assuming we make some validations that would require querying the database (which I still think are very important). Assuming we have to make these validations, assembling the transaction on the wallet service instead of the wallet isn't any more complex (we would have all the fields on hand already, it would just be a matter of serialization). Probably just more convenient.
If we don't make any verification, then we don't have to make any queries, then actually is less complex. So in the end it seems to be a matter of simplifying for the sake of skipping these checks. Which I can totally accept for a proof-of-conecept implementation, but I think it's bad for the design of what it should be.

The wallet client or the tx-mining-service could have a bug, or there could be some exploit possible to sending a custom transaction through the wallet service, which could be cheaper than setting up a fullnode (but I'm not really sure on this).

I think this is the situation we already have and no issue has ever been raised. People can just use our full node APIs to push any tx.

I think my bias is that I'm assuming we make some validations that would require querying the database (which I still think are very important). Assuming we have to make these validations, assembling the transaction on the wallet service instead of the wallet isn't any more complex (we would have all the fields on hand already, it would just be a matter of serialization). Probably just more convenient.
If we don't make any verification, then we don't have to make any queries, then actually is less complex. So in the end it seems to be a matter of simplifying for the sake of skipping these checks. Which I can totally accept for a proof-of-conecept implementation, but I think it's bad for the design of what it should be.

To be honest, I'm not sure what validations we need to make. Could would give some examples?
In the end, if the wallet wants to send some "weird" tx and it owns the UTXOs, it can do it. There's nothing the wallet-service can do to prevent this. That's why I don't think it's a big problem if the service just acts as a proxy in the final send, receiving the full tx. As long as this cannot leave the db in some inconsistent state, I really don't think it's a problem.

I agree with @jansegre that the wallet service should validate the transaction and only propagate if it matches the tx proposal. Otherwise, there might be possible attacks.
If a new transaction is received from the full node, and this transactions spends an output associate with a tx proposal, we should mark that tx proposal as invalid and log it, unless this transaction was sent by the wallet service.
If the user would like to change the chosen utxo, the user must update the tx proposal. Otherwise, the tx proposal will decline to propagate the tx. If the user propagates directly to a full node, the tx proposal will be marked as invalid.
I don't mind what is the best format to receive the transaction from the user. A JSON is always easy to read and debug but we can receive the serialized transaction as well.

I agree with @jansegre that the wallet service should validate the transaction and only propagate if it matches the tx proposal. Otherwise, there might be possible attacks.

Do you mean validating inputs and outputs? Or just that there's a valid proposal id? And what attacks are there?

In the end, if the wallet wants to send some "weird" tx and it owns the UTXOs, it can do it. There's nothing the wallet-service can do to prevent this. That's why I don't think it's a big problem if the service just acts as a proxy in the final send, receiving the full tx. As long as this cannot leave the db in some inconsistent state, I really don't think it's a problem.

Why should the wallet propagate garbage txs? This could potentially open the door for some DoS if somebody finds a tx that takes long to validate on the node. If this isn't even possible in the first place, a whole class of possible attacks is prevented.

And what attacks are there?

Not any in particular is known right now. But I'd rather not leave the door open. Again, my point is not that this has to be the actual PoC implementation, but the target design. I'm OK taking a shortcut to implement it faster and get it tested.
For me we should have set a few higher level objectives and premises to the wallet service. One of them being shielding the fullnode as a security measure, currently we are exposed, and that is definitely not good. I'd rather have anyone that wants a fullnode API to run their own fullnode. I don't see why we should give up on this just because of one endpoint.

Why should the wallet propagate garbage txs?

But the wallet-service would only propagate txs with a valid proposal-id. Given that it'll be a random string (probably UUID), it's not easy to guess it.

Again, my point is not that this has to be the actual PoC implementation, but the target design. I'm OK taking a shortcut to implement it faster and get it tested.

This is a good point. I might actually go with this design right now since it's easier while we debate.

But the wallet-service would only propagate txs with a valid proposal-id. Given that it'll be a random string (probably UUID), it's not easy to guess it.

That's exactly the same as a CRSF. It's certainly better than no protection at all, but the content of a proposal submit would still be forwarded unmodified and most importantly, unverified to the fullnode. A 10MB transaction is invalid, but if we accept a request that large (there should be a max-body-size on the HTTP frontend, if we use one, hopefully we do) we would be forwarding that to the fullnode. Could it be crafted in a way that takes too long for the node to verify before rejecting? I don't know, if it could, this could lead to a DoS. Assuming we have more wallet-service workers than fullnodes, to improve scalability, this would impact the wallet service as a whole. This is a threat that can be avoided just by only forwarding well formed data to the fullnode. To me this is just as important as doing input sanitization when querying a database. Now a days most databases frameworks have some form of automatic sanitization, but we don't have anything like that for our fullnode.

The tx you're saying could be used in the future for an attack is:

a valid tx, structurally;
takes the node a long amount of time to validate;

If that's the case, it seems to me the problem is that we have such a tx and we'll need to correct our full node code so it doesn't happen anymore. I mean, no tx such take long to verify. Even if we prevent it from being done in the wallet-service, the same can be achieved by faking a synced node and sending this tx.

The tx you're saying could be used in the future for an attack is:
* a valid tx, structurally;

Sure but this doesn't even have to be the case. Because the "passthrough" proposal doesn't even check for structural validity.

* takes the node a long amount of time to validate;

If that's the case, it seems to me the problem is that we have such a tx and we'll need to correct our full node code so it doesn't happen anymore. I mean, no tx such take long to verify. Even if we prevent it from being done in the wallet-service, the same can be achieved by faking a synced node and sending this tx.

That's all good until it happens. And we'll have a potentially unstable and DoS'd wallet service until we fix and deploy the node. I'm not arguing this isn't a bug that has to be fixed in the node. It definitely has to. I'm arguing a design that has this whole is inherently more vulnerable to attacks (and also silencing bugs) than one that doesn't.

Sure but this doesn't even have to be the case. Because the "passthrough" proposal doesn't even check for structural validity.

If we reassemble the tx in the wallet-service before sending, as I proposed, that'd do it.

And we'll have a potentially unstable and DoS'd wallet service until we fix and deploy the node.

Not necessarily. We can probably "understand" this type of tx and specifically block it on the wallet-service. But I do agree that it's one less attack surface.

Sure but this doesn't even have to be the case. Because the "passthrough" proposal doesn't even check for structural validity.

If we reassemble the tx in the wallet-service before sending, as I proposed, that'd do it.

I think I missed that part. But what would be the meaning of reassembling without using the fields from the database? If it's just checking the structural validity, then "reassembling" just means validating (but only the structure) in this case, no? And why not validate more than just the structure, the cost is an additional query (or requesting additional data on the "proposal id" query).

But what would be the meaning of reassembling without using the fields from the database? If it's just checking the structural validity, then "reassembling" just means validating (but only the structure) in this case, no?

Yes, that would only be making sure we're sending a tx that is structurally valid. That means no one can attack by sending "garbage".

And why not validate more than just the structure, the cost is an additional query (or requesting additional data on the "proposal id" query).

It was not clear to me the benefits of doing it. The cost might seem just an additional query, but there's always more work behind, making sure the databases are consistent. Therefore, the simpler design seemed more attractive to me.
However, the suggestion I gave to reassemble the tx is probably very close to doing the full tx assembly on the service, as you've been saying. So I'm starting to agree with your point of view, although not yet 100% convinced 😆

I am just trying to understand what are the downside (besides more work but not too much) of validating the the inputs/outputs of the tx being sent matches the ones from the tx proposal, which is something I said in my last comment I think we should do.
After reading all other comments I've seen good arguments to do this and the only reason not to do is that it would be more work. I think we should do it and in the first version of the tx-proposal code, I don't think it would increase a week of work, so it's pretty okay.

the only reason not to do is that it would be more work

My point is not more work, is more complexity in the code for a benefit that's not clear to me.

Ok, so after all the debate, I'll implement it as Jan suggested. I'll add fields tx_proposal and tx_proposal_index to the UTXO table and create a new tx_proposal_outputs table, to keep track of the outputs.

Introduction
The hathor-wallet-service will be used as backend for all official Hathor's wallets.
Sync between full node and service is being designed here: PR https://github.com/HathorNetwork/hathor-wallet-service/issues/8.
Architecture

┌──────────────┐       ┌─────────────────┐       ┌───────────────┐
│              │       │                 │       │               │
│  Full node   │──────▶│  Tx Processor   │◀─────▶│   Database    │
│              │       │                 │       │               │
└──────────────┘       └─────────────────┘       └───────────────┘
                                                         ▲
                                                         │
                                                         ▼
                                                 ┌───────────────┐
                                                 │               │
                                                 │  API Service  │
                                                 │               │
                                                 └───────────────┘

The service will be implemented using AWS services.

DynamoDB: Database
AWS Lambda: Tx Processor and API Service
AWS Gateway API: HTTP and Websocket endpoints
AWS SQS: Queue service
AWS Aurora Serverless

Services
Tx Processor
Receive transactions/blocks from the full node and update the database. It will receive two types of messages: NEW_TX and UPDATE_TX. The latter is used to notify a change in the tx status (from executed to voided or otherwise).
It must keep the balance and transaction history of all addresses, even the ones not designated for a wallet. In the future, we may include an API to query the address and tx history for any address. We would also easily know the richest addresses.
When a new tx arrives or a tx is updated, it must update the database for all affected wallets.
API Service
Load from database and return to the wallet. The idea is to do the least number of calculations here. All things should be pre-computed by the TxProcessor. Only locked balances/authorities may trigger computation on the APIs.
Maintenance Tools
For each table, we should have a verification tool that checks its consistency. This tool should also have a parameter to recalculate the records of a given table. These tools will be integrated in our monitoring and alert system.
Database
- Wallet
  - Id
  - XPubKey
  - MaxGap (default: 20)
  - Status (creating, ready, error)
  - CreatedAt
  - ReadyAt

- WalletBalance
  - WalletId
  - TokenId
  - UnlockedBalance
  - LockedBalance
  - UnlockedAuthorities
  - LockedAuthorities
  - TimelockExpires
  - Transactions

TimelockExpires contains the earliest timestamp at which any locked balance or authority will become unlocked.
- WalletTxHistory (for all transactions)
  - WalletId
  - TokenId
  - TxId
  - Balance (positive: received / negative: sent)
  - Timestamp

- Address (for all addresses, even if WalletId is null)
  - Address
  - Index
  - WalletId
  - NumberOfTransactions

Index indicates the derivation path index used for this address. Both Index and WalletId will be null until the corresponding wallet is initialized in the service.
- AddressBalance
  - Address
  - TokenId
  - UnlockedBalance
  - LockedBalance
  - UnlockedAuthorities
  - LockedAuthorities
  - TimelockExpires
  - Transactions

Same idea as WalletBalance table.
- AddressTxHistory
  - Address
  - TxId
  - TokenId
  - Balance (positive: received / negative: sent)
  - Timestamp

- UTXOs
  - TxId
  - Index
  - TokenId
  - Address
  - Value
  - Authorities
  - Timelock: at which time this becomes unlocked
  - Heightlock: at which height this becomes unlocked (for blocks)
  - Locked: boolean indicating if it is locked
  - TxProposalId: the tx proposal id, if applicable
  - TxProposalIndex: the input index on the tx proposal for this UTXO, if applicable

- TxProposal
  - Id
  - WalletId
  - Status (open, sent, cancelled)
  - CreatedAt
  - UpdatedAt

- TxProposalOutputs
  - TxProposalId
  - Index
  - Address
  - TokenId
  - Value
  - Timelock

This table is used to store the suggested outputs for a tx proposal. The index indicates the output index.
- Token
  - Id
  - Name
  - Symbol

Flow from wallets

Client creates a wallet (wallet_id = sha256d(pubkey)).
Client polls the wallet until it is ready.
Get balance and tx history for a token.
Create a tx proposal.
Sign the inputs of the tx proposal.
Resolve the tx proposal.
Propagate the tx proposal.

Events from Full Node

New tx or block.
Update tx or block.

API for the Wallet
Authentication
All requests must be signed by m/1/1 of the wallet's xpriv.
Websocket:

Wallet must authenticate once after connection;
Wallet must received a balance update message every new tx, so it can update its balance and tx history;

POST /wallet: Create a new wallet
When the wallet is created, the following tasks must be executed:

Mark its addresses in the Address table.
Calculate the balance of all its tokens and store in the AddressBalance table.
Create the WalletTxHistory records from the AddressTxHistory.

After requesting the creation of the wallet, the client must polling to verify when it is ready. It may take a few seconds to be ready. If an error occurs, the user must have the choice to check status again or reset the wallet.
Arguments:

XPubKey

Returns: { walletId, status }
GET /wallet: Get status of a wallet
Arguments:

WalletId

Returns: All fields of the wallet, including its status.
GET /addresses: Get wallet's addresses
We can run one query in the Address table filtering by a given walletId.
Open question: Should we paginate?
Arguments:

WalletId

Returns:

List of addresses

Address
Number of transactions
Index

GET /balance: Get wallet's balance
Run one query in the Balance table filtering by walletId (and tokenId if the argument is passed).
Arguments:

WalletId
TokenId (optional)

Returns:

List of balances:

Token { Id, Name, Symbol }
Number of transactions
Balance { Unlocked, Locked }
Authorities { unlocked: { mint, melt }, locked: { mint, melt } }
LockExpires

If TokenId is received, it will return only the requested token (list with 1 element).
GET /txhistory: Get wallet's transaction history
Arguments:

WalletId
TokenId
Skip
Count

Returns:

Skip
Count
List of transactions

Format: { txId, timestamp, balance }

POST /txproposal: Create a new tx proposal
When a tx proposal is created, it should mark the used UTXOs. They do it by assigning a TxProposalId and TxProposalIndex to them. So, next tx proposal will skip them avoiding double spending. We can optionally create a scheduled job to cancel all tx proposal opened for more than X minutes.
Arguments:

Input selection algorithm (optional) (e.g. best match, from highest, from lowest)
Outputs [[addr1, amount1, token1, timelock1], [addr2, amount2, token2, timelock2], ...]
Inputs (optional) [[TxId1, Index1], [TxId2, Index2]]

Returns:

Tx proposal

Id
Outputs: [{ address, value, token, timelock }]
Inputs: [{ txId, index }]
Tokens: [tokenId1, tokenId2, etc]

The wallet can use this info to assemble the tx and mine it, probably using the tx-mining-service, and propagate the tx using the API bellow.
PUT /txproposal/{txProposalId}/: Send the tx
Arguments:

TxProposalId
List of input data [ inputData0, inputData1, ...] (the signatures)
List of parents
TxWeight
Nonce
Timestamp

Returns:

Success or error message

DELETE /txproposal/{txProposalId}: Cancel a tx proposal
When a tx proposal is closed, it should remove all locks from the UTXOs used by it.
Arguments:

TxProposalId

Returns:

Success or error message

As we discussed, closing this issue in favor of #10, where the updated design was posted. This design here is already too long and we'll keep it for records.


Modem charge mAh with different configuration

I noticed that when ral_get_lora_rx_consumption_in_ua is called. rx_boosted is always set to false.
Our PCB uses the sx1261 powered with a 1.8V power supply, the implementation to track consumption only uses the table for a 3.3V power supply.
Will there be an implementation in the future to track the power using rx_boosted and 1.8V?

This limitation is confirmed and will be fixed in the next SWL2001 release.


artpar 2nd attempt

Check List:

[x] Tests pass (./test.sh <username>)
[x] All formatting changes by the build are committed
[x] Output matches that of calculate_average_baseline.sh

Execution time: 00:5.127
Execution time of reference implementation: 3:08.77

[base]                   183.28s user 5.63s system 100% cpu 3:08.77 total
[memory mapped+parallel] 167.33s user 5.04s system 630% cpu 27.321 total
[above+vector]           157.12s user 3.43s system 670% cpu 23.958 total
[update]                 78.74s user  6.10s system 680% cpu 12.459 total
[b7385ed]                65.03s user  5.68s system 664% cpu 10.634 total
[dda0f36]                61.29s user  5.36s system 727% cpu 9.163 total
(^ previous pr)
[4e6dc45]                35.99s user  5.96s system 680% cpu 6.168 total
[cbdb861]                27.58s user  5.66s system 648% cpu 5.127 total

There are test failures. Please run ./test.sh artpar and make sure no differences are reported.

I have verified that all tests pass now (there are considerable changes since the last commit, i tried to reproduce test failures on that commit but didn't get any fail). Also verified the other two check marks again as well

00:08.398 now.


Error: Usage of old X[Twitter] logo.

The website is currently using the old logo of X (formerly Twitter), which might cause ambiguity among the users.
current view:

I want to change the logo of twitter to the updated one. This will remove any ambiguity among the users.

I would like to work on this issue.
please assign the labels of gssoc extd and hacktoberfest accepted and level.

Hi @ABHISIKTAcommits, go ahead it's yours!
Do remember you have 3 days to complete the issue.
And if you like the project do give as Star :star2:

@Suraj-kumar00 please add the label of hacktoberfest-accepted 😃

@Suraj-kumar00 please add the label of hacktoberfest-accepted 😃

Don't worry this contribution will be added the the hacktoberfest because this repository has hacktoberfest label.

Hey @ABHISIKTAcommits, what is update on the issue?

sorry to say...im unable to work with this issue... @Suraj-kumar00

Oh okay, I though you can do that! Any way I'm unassigning you so that other contributor can work on this!

@Suraj-kumar00  , Hello, I would like to work on this issue. Could you please assign it to me?

Hi @biswajit-sarkar-007, how have you been?
It's yours, do remember that you have 3 days to complete the issue!
And if you like the project please do consider to give the repo a star :star2:

@Suraj-kumar00  I am facing problem to set up your code in local machine. U can assign it anyone


open in wallet link not working as expected

It seems like there is some code running on the client side waiting for the connection, however the browser no longer have focus, so it doesn't send the credential until switch back to browser

So the deeplink takes the user to the BCWallet app and puts the web browser in the background. The wallet starts it's side of the connection but the demo can't answer because the javascript can't run while the browser is in the background. I'll need to move the connection logic to the backend to fix this issue.
As a temporary work around while I fix this: once you click the deeplink and the connection starts, navigate back to the web browser for a moment, and then back to BCWallet and you should see a credential offer/proof request


doesNotHaveElementsOfType

Check List:

Fixes #962
Unit tests : YES
Javadoc with a code example (API only) : YES

@joel-costigliola thx for your feedback! I just updated the pr.

Thanks @epeee ! I've just enhanced the error message by showing the unexpected types and the corresponding element values.


Add a simple DeviceSpec class.

This is a straightforward port from device_spec.py.
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/framework/device_spec.py

Do you know if the Auto value stuff will be upgraded to use Records (https://openjdk.java.net/jeps/384) when it comes out of preview? As otherwise it might be preferable to use plain Java classes that we can convert into records when support comes (as I think it's supposed to be a binary compatible change).

Do you know if the Auto value stuff will be upgraded to use Records (https://openjdk.java.net/jeps/384) when it comes out of preview? As otherwise it might be preferable to use plain Java classes that we can convert into records when support comes (as I think it's supposed to be a binary compatible change).

I don't :)
Though I'd imagine it to be a fairly easy port over?
Also taking a look at the record, it doesn't seem supporting a customer builder or stuff like that? In our case, the values are optional - the default null value builder seems cleaner ATM.

Yeah, you have to specify the builder by hand, those aren't part of the language support for records. Though you can have multiple constructors for a record provided they call into the canonical constructor.

Thanks for the PR @hthu ,
I would suggest having a builder that does not depend on any external library. tensorflow-core-api is a low-level library that should bring a limited number of transitive dependencies as it increases the risk of incompatibilities if the user also depends on these other libraries.
We had unfortunately no choice with protobuf-java but in your case, while auto-value provides an elegant way to build such builder, it can be easily replaced by a simple custom builder.

Hi @hthu , do you intend to make changes on this PR or leave it as is? Do you have any feedback to provide based on the suggestions @Craigacp and I have made?

Oops, totally forgot about this.
Will drop autovalue deps and deal with it over the weekend!

Updated to a version without auto.
PTAL.

Updated to a version without using auto.
PTAL.


Node/Web sdk unprotect gives a different token that causes errors when making subsequent calls

Reproduction steps

Sign in with AccountDetails (name and email)
Verify email with code sent to email
Make any call on the wallet or credential services (simple one I used is getWalletItems)

Reproduction Code
The commented out code is what's broken. The code currently running works fine.
let main = async () => {
  let accountService = new trinsic.AccountService();
  // let base64Profile = await accountService.signIn(new trinsic.SignInRequest().setDetails(new trinsic.AccountDetails().setEmail("michaelblack117@gmail.com").setName("Michael Black")))
  let base64Profile = await accountService.signIn(new trinsic.SignInRequest())
  accountService.options.setAuthToken(base64Profile);

  // let code = await askQuestion("Enter the security code sent to your email:\n");
  // let p = trinsic.AccountProfile.deserializeBinary(Buffer.from(base64Profile, "base64"));
  // let profile = await accountService.unprotect(p, code);
  // accountService.options.setAuthToken(profile.getAuthToken_asB64())

  let walletService = new trinsic.WalletService(accountService.options);

  let searchResponse = await walletService.search();
  console.log(searchResponse.getItemsList())

  // let credentialService = new trinsic.CredentialService();
  // credentialService.options.setAuthToken(profile.getAuthToken());

  // let templateService = new trinsic.TemplateService();
  // templateService.options.setAuthToken(profile.getAuthToken());

  // let issueRequest = new trinsic.IssueRequest().setDocumentJson(JSON.stringify(unsignedCredential));
  // let issueResponse = await credentialService.issueCredential(issueRequest);
  // let signedCredential = issueResponse.getSignedDocumentJson();

  // console.log("cred", signedCredential)
}

Let me nose around this one. I assume it's happening on prod?

Yes on prod specifically. I ran into on staging or dev too at one point but not consistently

Can reproduce this, investigating.

Found the issue, fix coming shortly. Yay UTF-8 encoding assumptions! 🤦


Run chisel test suite with Treadle

This PR introduces a dependency on Treadle for backend testing.
Looking for comments on implementation and concept.
Type of change: Testing enhancement
Impact: no functional change to chisel
This could enable running unit tests to go much faster, probably about 10x.
This also helps make sure Treadle has a high fidelity to verilator.
Development Phase:  implementation
Release Notes
Chisel test suite tests can now be run using Treadle  (manually).
This PR presents a use case for the Issue on global settings

I'd love to use treadle for testing, I think it'll vastly speed up how long the Chisel CI tests take. One ask I have though is that it's relatively easy to make it use Verilator-only. Just for sanity checking we should probably run all the tests using Verilator as part of a nightly regression or something. For normal CI though, I'd love to switch to treadle so that they run faster. on FIRRTL CI, the firrtl tests take 5-7 min while the Chisel tests take 28 minutes.

Nightly run, do verilator. For general CI tests, run treadle. Thus, we should make it easy to switch backends.

I think this needs rebasing, I doubt you're trying to PR 109 commits

In general, I think that using a persistent-state mechanism (such as the filesystem) basically ephemeral communication is kind of a red flag:

Most importantly: because state is persistent, you have to be really thorough about initializing and clearing state, or you might have dependencies between runs causing non-intuitive behavior.
Changes to the filesystem may also be detected by other tools - for example, how would this interact with git? Should we also be concerned with the edge case of files being modified during test runs (admittedly a somewhat unlikely case, and assuming the file is an intermediate only)?
The mechanism isn't immediately obvious (you basically would have to locate the particular piece of documentation), especially if you were trying to debug behavior. Potentially could be mitigated with detailed logging / printing, so it's clear where the system is looking in deciding the backend - kind of a form of on-demand documentation.
Probably subjective, but it feels hacky.

One question that may be worth asking, is who is the target audience?

General RTL engineers, for any Chisel project: maybe we should emphasize mechanisms that are analogous to what RTL tools today do - for example, environment variables? Command line flags? If we're fine with a dependency on ScalaTest, it provides its own test environment map, that you can read during the test case and define on the sbt shell. I think that last one (possibly with a generalization / interface to multiple test frameworks) is the cleanest solution, and uses an "expected" interface.
Chisel developers / CI server, not exposed as a "public" interface for Chisel-based projects to use: can definitely be a bit more rough around the edges. Environment variables might be fine, though still raises the problem of persistent state. The file might be acceptable - it's still a potential gotcha, but affects much less people. Or maybe something as simple as changing the code that returns the default backend - where the usage is super hacky, but architecturally eliminates the risk of unintuitive behavior from most users?

@jackkoenig I rebased again, not sure how I ended up with that many commits
@ducky64
Using the file system has been red-flagged, but I believe it to be the best compromise

Environment variables are equally problematic as files, and are harder to use in an IDE
The file system is relatively clean, I have provided sbt tasks for setting and clearing the file
The file used has been added to .gitignore so that it is is unlikely to be accidentally passed up to github
Tests will not run if the file is in a messed up state.
The control file could conceivably be modified during test runs but I don't see it as that likely

The main target audience is developers working on chisel3

During coding-debugging loop this mechanism provides a lightweight comprehensible way to switch to a faster running test suite

The secondary target is having variations in testing strategy during different CI runs.

The sbt tasks created provide a standard mechanism for controlling this.

The file based approach is not immediately obvious.

I will be providing website documentation on why and how one would use this
There should be more documentation for internal developers and that would be a natural place to document this.
I don't like the idea of changing the code to get different backends, that seems way more hacky than this approach.

I have been trying to get this basic goal of treadle running the chisel3 tests for over a year, in all that time no more elegant control mechanism has been proposed. The other major benefit of getting this in is it strengthens treadle testing and fidelity to verilator. As we are pushing treadle as the default backends for our unit testing frameworks this is a pretty important goal

So thinking about it more fundamentally, here's how I see the trade-offs:

Under the current PR'd code: sbt file is the user-facing interface, files are used as an intermediate. Pros: reasonable interface. Cons: hacky (and potentially leaky - though in uncommon circumstances) implementation.
Changing code directly. Pros: very clean, lightweight implementation, basically no side effects. Cons: hacky interface.

But crucially, to change the backend, in both cases the user needs to modify a version-controlled file (.sbt file in the first option, .scala file in the second option) and make sure to not commit the changes. Yes, changing the .sbt file is a bit cleaner and nicer, but I don't see that as so different from changing the source file. However, the difference is that in option 1, there needs to be additional (potentially leaky) plumbing and complexity, whereas that isn't the case in option 2. Both are hacky, but for an internal developer tool, I'd argue option 2 is the least-worst solution.
I'll also put in my 2c in that I don't think documentation alone is sufficient, nor should it be the first line of defense. Getting started guides are great, but beyond that, documentation needs to be discoverable (one typically does not read through a long manual or crawl websites for fun, it's probably more search for one specific task that I want to get done - or worse, search for symptoms of some weird bug and hope there's like a StackOverflow link). Even better is if documentation isn't needed in the first place, either because the functionality is "obvious", or we're re-using structures (precedent) that are common or expected in the field.
I think there's also a solution space between options 1 and 2:

Is it possible to get SBT data directly into Scala? For example, a quick search turned up sbt-buildinfo, will that do what you need? It would also eliminate the raw file IO functions (to some degree, implementation details) currently in build.sbt, replacing them with a more structured version.
If you want to avoid changing version-controlled files completely, there's the option is just having a configuration file as the interface, instead of the configuration file as an intermediate from SBT (which kind of seems like a worst-of-both-worlds solution). There's at least precedent in that some logging frameworks are configured from a separate .xml file - though I think this is kind of cumbersome. I don't know if there's a general underlying configuration framework that can be re-used.

I definitely agree that it's valuable to test with treadle (possibly / probably even having treadle be the default backend), but I'm not convinced this is the right solution.
If you wanted to widen the solution space, it might also be worth considering whether the common use case will be to switch backends for all tests, as opposed to having a default backend for most tests (that test chisel functionality), and then a few tests that do sanity checks on specific backends. Under this philosophy, where it's very rare for developers to change the backend for all tests, option 2 (from way above) makes a lot more sense. It still might be worth having CI run all tests against all backends, but adding the complexity to the CI setup might be less worse than adding complexity in chisel base.

@ducky64 said

But crucially, to change the backend, in both cases the user needs to modify a version-controlled file (.sbt file in the first option, .scala file in the second option) and make sure to not commit the changes.

This is NOT the case. There is no requirement that the build.sbt be changed in order to switch backends. My changes to sbt provides new tasks that can be run from CI scripts that can set the backend using the external file.
I'll also put in my 2c in that I don't think documentation alone is sufficient, nor should it be the first line of defense. Getting started guides are great, but beyond that, documentation needs to be discoverable (one typically does not read through a long manual or crawl websites for fun, it's probably more search for one specific task that I want to get done - or worse, search for symptoms of some weird bug and hope there's like a StackOverflow link). Even better is if documentation isn't needed in the first place, either because the functionality is "obvious", or we're re-using structures (precedent) that are common or expected in the field.
I think there's also a solution space between options 1 and 2:

Is it possible to get SBT data directly into Scala? For example, a quick search turned up sbt-buildinfo
We already use BuildInfo for versioning in Firrtl, it's an interesting tool  but it does not change the fact that the build.sbt file must be changed to change the backend. Given that I don't see that it provides any better solution to the problem.

Discovery of this feature is a small problem but it is not really that different from the discovery issues of, for example, how to turn on debug of the firrtl passes. Only a small pool of people are at this level of development and they ask questions. Tips like these are often just word-of-mouth.

So if we wanted to draw bright lines between interface and implementation, both the project .sbt and the file it reads are considered user-facing interfaces - essentially, two ways to do the same thing (possibly depending on use case - which I assume would be documented somewhere)? I'm definitely not a fan of this philosophy - yes, Scala does do it, but I think there's a lot of drawbacks unless there's strong driving use cases and guidance on which to use when.
As for documentation / discovery: really a question of trade-offs. I'd argue that if a elegant architectural fix is reasonably implement-able and provides gains in usability, then it's worth examining. Is there a reason something a solution like sbt-buildinfo won't work?
In general, I think it's worth thinking clearly about who's going to be using this, what the interface cleanliness should be, the trade-off of implementation cleanliness vs. dev time, and separation of concerns. From your reply, it looks like the driver being those functions in the .sbt is just CI hooks, which might be used just a handful of times, ever, only in the Chisel CI setup. Subjectively to me, I'd argue that those functions just add complexity to the build.sbt file (think from the standpoint of a new user - perhaps reading to chisel build.sbt in integrating to another project) - yes, can be alleviated with surrounding documentation, but maybe it's better to just move in to the CI config?
To keep progress moving, it might be prudent to split this into two PRs: one that adds the non-contentious internals (treadle .sbt dependencies, treadle runner, global default backend, excluding file reading) that could be merged sooner, while splitting the more contentious interface parts into another PR?

@ducky64 You have come up with the same solution that I did last night. I will split these PR's and we can discuss the issues separately.
I will note in passing the following.

This features is for internal developers of Chisel3 and Continuous Integration systems.
For the latter in particular the ability to switch backends should not require editing of source controlled files.
I have experimented with BuildInfo, and talked to Jim about it and short of using it to read a file or environment variable, it requires editing the build.sbt (or mill equivalent)

FWIW, I tried to do this with environment variables and sbt build targets. With sbt you can set a variable with envVars in Test := Map("BACKEND" -> "verilator"). However, I couldn't sort out how to have two different testing build targets that would both inherit whatever Test does, but set different environment variables. That would theoretically enable my ideal API for this: sbt treadle:test or sbt verilator:test without filesystem state. However, my sbt skills are just way too weak and I couldn't sort out how to do it.

Replacing this with PR #1483 which just creates the Treadle backend mechanism
And a PR to be numbered later to provide a switch for this feature


fix: public initializers

@xiaoluSF @cezres You guys might want to try the develop branch with CKB's edge branch. There're quite some structure changes.


VTK version problem

The following configuration files were considered but not accepted:
I TRY VTK 8.1.0, 8.1.1, 8.2.0 version, but I still get this error. What's the matter?

The following configuration files were considered but not accepted:
D:/VTK-8.1.1/build/VTKConfig.cmake, version: 8.1.1 (64bit)