revault / revaultd Goto Github PK

darosior
20:07
Hmm we should have a "i already signed" state don't you think? Weird to still have to acknowledge funds on homepage after succesful signing

edouard
09:43
yes, I can check for each vault not spent if the presigned transactions are signed but If there are 6371 vaults it would take time to load
maybe once revocationtxs is done, we update a column in vault table with: revocation_txs_signed: true
and the column has a field is in the resource of the listvaults call

darosior
09:45
That's basically equivalent to having a new state

edouard
09:46
yes, do we both agree that a new vault status is too much ?
two other column in the table could do the job:
revocation_txs_signed unvault_tx_signed

darosior
09:55
Well i feel like the opposite (as usual lol), adding more columns seems overkiller than a new state. But the number of state is bloated. I'm thinking of a way to do it without DB
Let's put an issue for now

We currently use 6 blocks ™️ as a placeholder, but we should give this more thoughts.

• address
• derivation_index
• updated_at

check doc/api/listvaults

Users will have different use-cases, and we should need to adapt to their requirements.

Typically:
"Accounting view" with receive/spend
"transaction view" as in Bitcoin transaction
"detailed view" with fees, cancel, change, etc

We also should have a tool to extract a CSV for accounting purpose.

revaultd is creating a directory (the "datadir") and some files (the database, the UNIX domain socket for RPC) on the host OS. We want to setup some basic OS permissions on these files (why would another user get allowed to modify our database, or use our RPC socket?). These permissions are set up at creation time in different places:

• https://github.com/re-vault/revaultd/blob/68295908962abeb5b2e85ad6a4d3218273f27333/src/daemon/revaultd.rs#L205-L220
• https://github.com/re-vault/revaultd/blob/68295908962abeb5b2e85ad6a4d3218273f27333/src/daemon/database/actions.rs#L30-L49
• https://github.com/re-vault/revaultd/blob/68295908962abeb5b2e85ad6a4d3218273f27333/src/daemon/jsonrpc/mod.rs#L315-L328

However, they are only currently setup for UNIX systems, and Windows is special-cased to setup no particular restrictions. We need to setup some permissions also on Windows, but first we need to figure out how :)

Some guidance on this issue:

• It seems that the equivalent of permissions under Windows is to use ACLs. Maybe the stdlib has some way of abstracting it. Please don't introduce new dependencies just for this :).
• To decode the UNIX permissions set in the links above, we want rw only for the owner of the file for the database and the RPC and rwx (still only for the owner) for the data directory.

Since we run as a daemon, we get no backtraces for panic. Therefore i use this pattern everywhere we want to panic:

unwrap_or_else(|e| {
     log(e);
     exit(1);
});

Woudl be great to either make it a macro or setup fern to catch panics.

Another participant could make an external party send to the script derived from the 2**31 - 2 's key just to bother us. We need to handle this...

We currently have a basic queue which can fills until a certain limit. A very basic threadpool-like logic could help.

It's currently stored as a base64 string... Which makes no sense

That's what the doc specifies but we don't actually do it !

AFAICT only cookie-file based bitcoind RPC authentication is supported atm. RPC password authentication has many advantages like the use of multiple usernames and rpcwhitelists.

If we fail to communicate with bitcoind, we'd currently crash. Instead it would be nice to retry until a timeout (say 60s as default for actual users?) which would be configurable so we can fail early in functional tests.

A good place to implement it would be in
https://github.com/re-vault/revaultd/blob/1f6b2d9b640aaf8db1fd8c9c4b1ca2000d2984b1/src/daemon/bitcoind/interface.rs#L70-L83

It currently is hardcoded to be more agressive on regtest, we should instead specify it in the config

I think @edouardparis will need to know if the revaultd is configured for a stakeholder or a manager without actually parsing the config ?

As described in the doc https://github.com/re-vault/revaultd/blob/master/doc/API.md#listvaults

As it's exposed in a single server module, it think we could fuzz the mio_loop:
https://github.com/re-vault/revaultd/blob/3141908c8c1e847b2e76f9240c109734d1324599/src/daemon/jsonrpc/server.rs#L121-L125

We could use a custom MetaIoHandler that would not process the actual command but rather be used to acknowledge their reception. This would largely complement our unit tests that are only for now testing for simple situations and edge cases that we uncover as we go.

We could use the great libFuzzer integration by the rust-fuzz org.

In the unvaulttx call (as well as in the revocationtxs call for that matter) we might crash or have a network connection error just after storing the sig in db, resulting in us having the sig but the others not being able to fetch them.

At startup we could getsigs for our sigs to make sure the coordinator got everything already.

So i don't forget..

setup_bitcoind should just return a BitcoindError

can be both a manager and a stakeholder

We are trying to do too many things with  listtransactions. We need to split it into different purposes.

@edouardparis what do you need from the other side of the UDS ? :)

We denote vaults for which all revocation transactions are signed as being "Secure". That's poor terminology and we should find then use a new one..

On my sig sharing branch, i could reliably reproduce the error @edouardparis encountered for a while now. This has likely to do with the size of the request.

cargo run --bin revault-cli -- --conf config_regtest.toml revocationtxs 99924e3d562891b6b972e268a10a4ae4321280eb7b2759d0f1f86db500e82d04:1 "cHNidP8BAF4CAAAAAQvn7DOXXnPUZqa3v7gwctBN0yNMOagHYYDl+4Wq+2O9AAAAAAASAAAAAcbVmTsAAAAAIgAgy7Co1PHzwoce0hHQR5RHMS72lSZudTF3bYrNgqLbkDYAAAAAAAEBK7A4mjsAAAAAIgAgtYkRK0YZh1BcaCEDFv1qa3z/G3bl1zNYrXV2LYjQwhMBAwSBAAAAAQX9RwFSIQJm7Qi89u6RJ80TF1of0wGkdFBDUD+FV/9LvhhQx7TytiEClghIhOmp4rLxE40iu68WDFRWbj5cqP+qig/sMJV9KZRSrmR2qRTIPHfJailcIXqf1nObeNERfLzUp4isa3apFCrfSAV1plExbDv+7hUfsxciw6RhiKxsk2t2qRTEDwSFmw8jMC9oIjTvxaO5dXHly4isbJNrdqkUWNaeGHwKcz885RxsiARUuJKVNUmIrGyTVIdnVCECZEz54reP6wp1HlBQL1MKTL0LvaMCB3lgU5HnFlTdZsIhA87VXRIIvYxrQrEeKbqld3EcroMbOhKWYHxeXT7TZfScIQJiN/ZV879F/Wt6oA6RwmA9YVXxzAAeQPXkdmLZZcTHeSEDCjy8+/33Ei/n+oMDVMlW6mWV8tveIyhvA7wewMFoXKNUrwESsmgAAQHPViECZu0IvPbukSfNExdaH9MBpHRQQ1A/hVf/S74YUMe08rYhApYISITpqeKy8RONIruvFgxUVm4+XKj/qooP7DCVfSmUIQNm0V2PSlsko5OmMsfXZKKzs8mx1UNiCdw+ubxdg1/eEyECBuQo8Ehzc4QLzLpMuacbzMQNVJgNMVcTyPOOjkEL0IwhA9cfv8Yyxztmgl0Ayb8taZKgvcvXEWVTG/WGxITDbPtqIQNrT42dBZP0p/8u5vnKnC9G7aZEtwOm4P4tK+VK1xr2WVauAA==" "cHNidP8BAF4CAAAAAQQt6AC1bfjx0Fkne+uAEjLkSgqhaOJyubaRKFY9TpKZAQAAAAD9////AahxmjsAAAAAIgAgy7Co1PHzwoce0hHQR5RHMS72lSZudTF3bYrNgqLbkDYAAAAAAAEBKwDKmjsAAAAAIgAgy7Co1PHzwoce0hHQR5RHMS72lSZudTF3bYrNgqLbkDYBAwSBAAAAAQXPViECZu0IvPbukSfNExdaH9MBpHRQQ1A/hVf/S74YUMe08rYhApYISITpqeKy8RONIruvFgxUVm4+XKj/qooP7DCVfSmUIQNm0V2PSlsko5OmMsfXZKKzs8mx1UNiCdw+ubxdg1/eEyECBuQo8Ehzc4QLzLpMuacbzMQNVJgNMVcTyPOOjkEL0IwhA9cfv8Yyxztmgl0Ayb8taZKgvcvXEWVTG/WGxITDbPtqIQNrT42dBZP0p/8u5vnKnC9G7aZEtwOm4P4tK+VK1xr2WVauAAA=" "cHNidP8BAF4CAAAAAQvn7DOXXnPUZqa3v7gwctBN0yNMOagHYYDl+4Wq+2O9AAAAAAASAAAAAcbVmTsAAAAAIgAgy7Co1PHzwoce0hHQR5RHMS72lSZudTF3bYrNgqLbkDYAAAAAAAEBK7A4mjsAAAAAIgAgtYkRK0YZh1BcaCEDFv1qa3z/G3bl1zNYrXV2LYjQwhMBAwSBAAAAAQX9RwFSIQJm7Qi89u6RJ80TF1of0wGkdFBDUD+FV/9LvhhQx7TytiEClghIhOmp4rLxE40iu68WDFRWbj5cqP+qig/sMJV9KZRSrmR2qRTIPHfJailcIXqf1nObeNERfLzUp4isa3apFCrfSAV1plExbDv+7hUfsxciw6RhiKxsk2t2qRTEDwSFmw8jMC9oIjTvxaO5dXHly4isbJNrdqkUWNaeGHwKcz885RxsiARUuJKVNUmIrGyTVIdnVCECZEz54reP6wp1HlBQL1MKTL0LvaMCB3lgU5HnFlTdZsIhA87VXRIIvYxrQrEeKbqld3EcroMbOhKWYHxeXT7TZfScIQJiN/ZV879F/Wt6oA6RwmA9YVXxzAAeQPXkdmLZZcTHeSEDCjy8+/33Ei/n+oMDVMlW6mWV8tveIyhvA7wewMFoXKNUrwESsmgAAA=="

[2021-01-31][23:55:43][rpc][TRACE] Request: P0p/8u5vnKnC9G7aZEtwOm4P4tK+VK1xr2WVauAAA=","cHNidP8BAF4CAAAAAQvn7DOXXnPUZqa3v7gwctBN0yNMOagHYYDl+4Wq+2O9AAAAAAASAAAAAcbVmTsAAAAAIgAgy7Co1PHzwoce0hHQR5RHMS72lSZudTF3bYrNgqLbkDYAAAAAAAEBK7A4mjsAAAAAIgAgtYkRK0YZh1BcaCEDFv1qa3z/G3bl1zNYrXV2LYjQwhMBAwSBAAAAAQX9RwFSIQJm7Qi89u6RJ80TF1of0wGkdFBDUD+FV/9LvhhQx7TytiEClghIhOmp4rLxE40iu68WDFRWbj5cqP+qig/sMJV9KZRSrmR2qRTIPHfJailcIXqf1nObeNERfLzUp4isa3apFCrfSAV1plExbDv+7hUfsxciw6RhiKxsk2t2qRTEDwSFmw8jMC9oIjTvxaO5dXHly4isbJNrdqkUWNaeGHwKcz885RxsiARUuJKVNUmIrGyTVIdnVCECZEz54reP6wp1HlBQL1MKTL0LvaMCB3lgU5HnFlTdZsIhA87VXRIIvYxrQrEeKbqld3EcroMbOhKWYHxeXT7TZfScIQJiN/ZV879F/Wt6oA6RwmA9YVXxzAAeQPXkdmLZZcTHeSEDCjy8+/33Ei/n+oMDVMlW6mWV8tveIyhvA7wewMFoXKNUrwESsmgAAA=="]}YAAAAAAAEBKwDKmjsAAAAAIgAgy7Co1PHzwoce0hHQR5RHMS72lSZudTF3bYrNgqLbkDYBAwSBAAAAAQXPViECZu0IvPbukSfNExdaH9MBpHRQQ1A/hVf/S74YUMe08rYhApYISITpqeKy8RONIruvFgxUVm4+XKj/qooP7DCVfSmUIQNm0V2PSlsko5OmMsfXZKKzs8mx1UNiCdw+ubxdg1/eEyECBuQo8Ehzc4QLzLpMuacbzMQNVJgNMVcTyPOOjkEL0IwhA9cfv8Yyxztmgl0Ayb8taZKgvcvXEWVTG/WGxITDbPtqIQNrT42dBZ.
[2021-01-31][23:55:43][rpc][DEBUG] Response: {"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error"},"id":null}.

Will push a patch shortly

#78 further increased disrepancies between Windows and non-Windows support. It looks like the latest version of mio included the Windows support for UDSs and we could get rid of the Windows-only code.

When daemonizing we currently stay in the directory we were started in. It's fine for development but eventually we should chdir to ~/.revaultd (or whatever our datadir is) as if it is unmounted we should die anyways.

We need to truncate amounts to u32 right now..

Specify it in the doc first

Right now, we take the descriptors parameters from the config (xpubs, unvault csv) in order to create a Miniscript descriptor on first startup.
This is kind of a bridge between the pre-Miniscript era where users were used to just put xpubs and It Works ™️ and the Miniscript era as if it's not first start we'll refuse to start with a config that does not correspond to the Miniscript descriptor stored in DB.

Is it the right approach ? At the end of the reflexion i believe this is a question of how modular we would like to be. It's just that we could store the Miniscript descriptor in the config directly as they are expected to be generated in advance (during the ceremony).
It would also be less regular-user friendly, redundant with the database, and we can't just change the descriptors and get everything to work immediately..

So the question(s) to answer here is:

• Do we want to keep a configuration with a list of xpubs and parameters (such as the Unvault CSV value), or should we move to store the Miniscript descriptors right in the config ?

I only use grcov locally, giving access to coverall was silly

It's a pretty decent and maintained library, but i'm sure we can go with something simpler.

I think it makes much more sense. That's the only non-PSBT transaction..

We already happen to sometimes name the unvault_emergency an emergency_unvault, etc..

Maybe store the bestblockhash in the db and rescan listunspent if it's not anymore part of the chain

We could allow to set the miniscript descriptor from the configuration. We would in this case disregard all other configuration values (apart from our_pubkey) and just store the descriptor in DB. This should trigger a rescan too.

We should also add a getdescriptors RPC call. So the process would be:

• During setup, call getdescriptors and backup your descriptors
• If anything goes south you can re-import your backup descriptor by stuffing them in your config.

control.rs is getting large and we could separate "domains" of control better (eg signature sharing / checking, bitcoind interface, db interface, ..)

@edouardparis reported the following on MM:

[2020-12-11][12:10:23][mio::poll][TRACE] registering event source with poller: token=Token(4), interests=READABLE
[2020-12-11][12:10:23][rpc][TRACE] Request: {"method":"getinfo","params":null,"id":172306,"jsonrpc":"2.0"}.
[2020-12-11][12:10:23][revaultd][TRACE] Got getinfo from RPC thread
[2020-12-11][12:10:23][rpc][DEBUG] Response: {"jsonrpc":"2.0","result":{"blockheight":222,"network":"regtest","sync":1.0,"version":"0.0.2"},"id":172306}.
[2020-12-11][12:10:23][mio::poll][TRACE] reregistering event source with poller: token=Token(4), interests=READABLE | WRITABLE
[2020-12-11][12:10:23][revaultd::jsonrpc][TRACE] Writing response '"{\"jsonrpc\":\"2.0\",\"result\":{\"blockheight\":222,\"network\":\"regtest\",\"sync\":1.0,\"version\":\"0.0.2\"},\"id\":172306}"' for Token(4)
[2020-12-11][12:10:23][revaultd::jsonrpc][TRACE] Dropping connection for Token(4)
[2020-12-11][12:10:23][mio::poll][TRACE] registering event source with poller: token=Token(5), interests=READABLE
[2020-12-11][12:10:23][rpc][TRACE] Request: {"method":"getinfo","params.
[2020-12-11][12:10:23][rpc][DEBUG] Response: {"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error"},"id":null}.
[2020-12-11][12:10:23][mio::poll][TRACE] reregistering event source with poller: token=Token(5), interests=READABLE | WRITABLE
[2020-12-11][12:10:23][rpc][TRACE] Request: ":null,"id":172306,"jsonrpc":"2.0"}.
[2020-12-11][12:10:23][rpc][DEBUG] Response: {"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error"},"id":null}.
[2020-12-11][12:10:23][mio::poll][TRACE] reregistering event source with poller: token=Token(5), interests=READABLE | WRITABLE
[2020-12-11][12:10:23][revaultd::jsonrpc][TRACE] Writing response '"{\"jsonrpc\":\"2.0\",\"error\":{\"code\":-32700,\"message\":\"Parse error\"},\"id\":null}"' for Token(5)
[2020-12-11][12:10:23][revaultd::jsonrpc][TRACE] Writing response '"{\"jsonrpc\":\"2.0\",\"error\":{\"code\":-32700,\"message\":\"Parse error\"},\"id\":null}"' for Token(5)
[2020-12-11][12:10:23][revaultd::jsonrpc][TRACE] Dropping connection for Token(5)
[2020-12-11][12:10:25][revaultd::bitcoind::interface][TRACE] Sending to bitcoind: Request {

I can't manage to reproduce, so let's keep it around

a4690df was purely dumb, but i don't want to get back to 100% CPU..

So we don't bypass an import in case of a crash or w/e..

I currently use the "unload all other wallets" workaround, but that's hacky af

Tell it on the README.. v0.21

Do we need them ?

CI is sometimes failing randomly with

json.decoder.JSONDecodeError

on RPC calls.