Reconsider how transactions are signed

[nedsalk]

Current situation

This current implementation of adding witnesses gives a suboptimal developer experience when crafting a custom transaction. In order to sign the transaction, one has to do the following:

const resources = wallet.getResourcesToSpend(...);
const request = new ScriptTransactionRequest();
request.addResources(resources);
request.updateWitnessByOwner(wallet, wallet.signTransaction(request));

The above example has two issues:

1. If the transaction got modified in any way after signing (e.g. by adding a predicate resource), the signature would become invalid.

request.updateWitnessByOwner(wallet, wallet.signTransaction(request));
request.addResources(resources2); // the signature above is now invalid

2. Signing the transaction requires using the wallet twice: once as an address, and once to sign the transaction. This is a sign of incomplete design.

Proposal

I propose an alternative approach:

const resources1 = wallet1.getResourcesToSpend(...);
const resources2 = wallet2.getResourcesToSpend(...);

const request = new ScriptTransactionRequest();

request.addResources(resources1);
request.addSigner(wallet1);

request.addResources(resources2);
request.addSigner(wallet2);

Here, the signers wouldn't actually sign the transaction. They'd just be added to a list of signers that would be used in the transactionRequest.finalize() method described below.

When the time comes to send the transaction, the current implementation of sending the transaction would force us to approach this redesign in two steps. First, we'd leave the Provider.sendTransaction(transactionRequestLike: TransactionRequestLike) interface and finish the signing within it:

// Somewhere within `sendTransaction`
transactionRequest.finalize(); 

This call to the new transactionRequest.finalize() method would return an immutable transaction request which we can identify with the ImmutableTx type for the sake of this discussion. This method would use the signers added via request.addSigner to sign the transaction appropriately, among possibly other things. It returning ImmutableTx would guarantee that nobody can modify the transaction afterwards.

The second redesign step would be to change the provider.sendTransaction method's interface to accept ImmutableTx, where the workflow with it would now look like this:

const request = new ScriptTransactionRequest();
// ... add resources
// ... add signers

const response = await provider.sendTransaction(request.finalize());

The benefit of this would be:

• Signalling to the user that they can modify their request until they want to send it. After they're finished building their transaction, it won't be modified.
• provider.sendTransaction wouldn't be mutating the transaction; it would only do some last validity checks (e.g. around gas price), and send it to the node as the user sent it.

This second step would probably take more time to implement. Some things that come to mind that might slow it down:

• The provider currently mutating the transaction. We have Make transaction dependency estimation an opt-in feature #1536 for it.
• sendTransaction on Account/BaseWalletUnlocked/Predicate might interfere and might have to be done beforehand.
• Because ScriptTransactionRequest is one of the building blocks of our SDK, it might span multiple packages.

Notes on the current implementation

Adding a resource to a ScriptTransactionRequest currently automatically adds an empty witness as well:

const request = new ScriptTransactionRequest();
request.addResources(resources);
console.log(request.witnesses);
// ['0x000000000...]

This is done to be able to correctly correlate inputs with their witnesses when the time for signing comes. Based on a search for the updateWitnessByOwner method of ScriptTransactionRequest, the actual usage of signing across our code base is happening only in BaseWalletUnlocked.populateTransactionWitnessesSignature.

Conceptually, however, there shouldn't be a need to add an empty witness when adding a resource. It's being added purely due to internal implementation details.

[danielbate]

Great proposal @nedsalk, I especially like the DX improvements here. Even just the nomenclature change and providing a flow that is more akin to what we already provide with resources will be an improvement.

An iterative approach is also appreciated. Phase 1, providing a signer that is called by the transaction request provides a good DX improvement with minimal interruption to the current implementation. I think we should exercise caution with phase 2, given the pre-requisites.

[Torres-ssf]

@nedsalk This seems to be a good improvement. I am just curious how each wallet/signer will be linked to the resources to be signed later.

I wonder if the current implementation was made this way for a specific reason.

The way that works right now, the wallet is used to sign the request at a very specific moment, and then it is not needed anymore.

Just thinking if we will not have any security problems by attaching an unlocked wallet to a transaction request, that will only be used later on.

[nedsalk]

This seems to be a good improvement. I am just curious how each wallet/signer will be linked to the resources to be signed later.

Each resource has an owner which corresponds to the public key of the wallet. The necessary information for this to work can be summed up in the interface:

interface TransactionSigner {
  publicKey: string;
  sign: (request: ScriptTransactionRequest) => string;
}

Because we could correlate between a signature and a resource via publicKey, the witnessIndex field of the resource can be updated accordingly to match the witness index of its owner.

---

I wonder if the current implementation was made this way for a specific reason.

@LuizAsFight and @luizstacio could chime in on that.

---

The way that works right now, the wallet is used to sign the request at a very specific moment, and then it is not needed anymore.

That moment has to happen at some point in every implementation. The sign method would be called by ScriptTransactionRequest.finalize(), after which it wouldn't be needed anymore as well. I don't really see the difference between a call to ScriptTransactionRequest.finalize and WalletUnlocked.populateTransactionWitnessesSignature.

Just thinking if we will not have any security problems by attaching an unlocked wallet to a transaction request, that will only be used later on.

Concerns around weakening security with this proposal would be higher if we were to e.g. implicitly pass in the wallet's private key directly into the ScriptTransactionRequest without the user's knowledge and have the ScriptTransactionRequest do the signing itself with that private key instead of requesting the signature from the wallet via the sign method. The proposal is proposing something similar to what e.g. Ledger does: give me a transaction and I'll sign it, but I won't give you my private key.

The important thing is that we don't pass the private key around so that no footguns around e.g. serialization can happen (e.g. the user for some reason decides to serialize ScriptTransactionRequest and we saved the wallet's private key in it without them knowing it, thus serializing the PK with it). I can't think of a way that this can be exploited. In the worst case where somebody has access to the memory of the program, they can extract the private key directly from WalletUnlocked instance anyways; we can't protect anyone from that.

[nedsalk]

The rust sdk's initial refactoring of transaction signing was implemented such that they were passing the private key to their TransactionBuilder and signing the transaction with it internally, but they've recently decided to implement what is, in essence, the same approach as the one from this proposal. That PR, which is currently open, can be found here. Their discussions could possibly resolve some questions that pop up but can also lead to some new questions in our context.