Return tx instead of psbt

[jrwbabylonlab]

What's not included in this PR:

1. The file structure and ways of organising the methods can be improved. I do not want to spend too much time on this as this is something we can pick up after testnet. Ideally we can make the main class to produce custom typed class instance in which it will contain methods such as toPsbt, toTransaction etc. The new class could also offer static method such as fromTransaction so that we could call toPsbt later on
2. Security checks/validations on the tx -> psbt or psbt -> tx conversions. I.e we can not blindly trust the data feed in, we always need to do sanity check before producing things for user to sign. Follow the conversation here

Verified in simple-staking:

1. Simple staking: https://mempool.space/signet/tx/31c1845d56f4b49c9d0b9b087a26163f643bf87fb737d82d3845fb4bd42f4942

2. Unbonding tx: https://mempool.space/signet/tx/bae3fe972b910e26fb78a17a988f89a9a064bfb0cc2a85798c3032a4e732bf15

3. Withdraw early unbonding tx: https://mempool.space/signet/tx/7caafd66357a2b6c9ea635884237fabd0f44e89bb1b30aa2223a203e576cb074

[gbarkhatov]

@jrwbabylonlab is it ready for review? If not - please ping when it is

[jrwbabylonlab]

@jrwbabylonlab is it ready for review? If not - please ping when it is

@gbarkhatov feel free to review. i have not had the chance to perform e2e test on this. but i won't merge it until i fully test it anyway. this is a big PR, so would be good to gather some feedbacks first

fyi @vitsalis @KonradStaniec

[gbarkhatov]

@jrwbabylonlab codebase needs to be formatted, some imports are redundant

[gbarkhatov]

This file requires formatting

[vitsalis]

I am a bit concerned with changing the core of this library to work with transactions instead of psbts. The natural flow is to first create a psbt and then convert it into a transaction by adding the necessary signatures. However, the library converts the flow to

1. Create transaction
2. Convert to psbt
3. Add signature and convert back to transaction.

In essence, we are adding an additional step in the entire flow in order to satisfy the UX requirement of avoiding an extra modal pop-up for converting the psbt into a transaction. I wonder whether there's a more compliant way to satisfy the requirement:

For example, convert the psbt into a transaction without having to sign it or just add a duplicate method for creating just a transaction. The first would be ideal, the second one would add some duplication, but would not affect the core flow.

[KonradStaniec]

I see this as a step in good direction for the library.

In general I think in final form, the library:

• should return most of the results in custom domain types when building transactions like: StakingTransaction, UnbodningTransaction etc. Those types do not need to be related to any BTC or psbt related types.
• it should provide helpers to convert our custom domain types into most popular types used in ecosystem i.e if caller want to sign there should be toPsbt function, if caller want to send tx to Babylon there should be toTransaction function etc

This would separate our domain logic (building staking scripts which are valid according to Babylon) from all the different custom formats caller can encounter.

[KonradStaniec]

when this would fail ?

Asking as tbh this weirds my out a bit. Imagine you are the caller of the function createStakingTransaction( and for some reason it throws some error about psbt. How do you supposed to handle it ?

When you call it you do not have even idea what is this PSBT thingy.

In my mind, the library code should build staking transaction and return it and all necessary data to the caller, and then it is totally up to the caller to convert this data into psbt when the caller want to do the signing.

[jrwbabylonlab]

I understand the concern, but I have a different perspective on this.

• The dry run serves as an early validation step. If the transaction we create cannot be properly converted to PSBT later in the flow, it's better to fail fast rather than after the user has already spent gas fees on submitting EOI. (Especially the unbonding tx/psbt path )

• Error msg should not be a concern. we could wrap it under try catch with custom error message and indicate the dry run in the method comment. The btc-staking-ts library's primary purpose is to facilitate BBN staking transactions. Its complete staking flow requires specific steps: building tx -> sending EOI -> converting to PSBT -> signing. Users implementing this library need to understand and follow this flow, hence error coming out dry run should be well understood.

• From a responsibility perspective, if we output a transaction that fails during PSBT conversion after the user has already submitted EOI and paid gas fees, that would be a our responsibility. By validating PSBT compatibility upfront, we protect users from wasting resources on transactions that would ultimately fail.
  In essence, rather than viewing this as just a transaction creation method, we should consider it as part of a larger, well-defined staking flow where PSBT compatibility is a critical requirement.

[KonradStaniec]

I think what I am getting at is that our library should always output valid bitcoin transactions, and every valid bitcoin transaction is convertible into PSBT.

Therefore it goes that this check is a bit redundant, as if we create valid btc transactions it will also be psbt complainant. The only case it would not happen is if we were have some pretty bad bug.

It is not blocking for me (a bit of redundancy never hurts) but having psbt's here pollutes a bit core of the library (imo).

One note:

users implementing this library need to understand and follow this flow, hence error coming out dry run should be well understood.

In essence, rather than viewing this as just a transaction creation method, we should consider it as part of a larger, well-defined staking flow where PSBT compatibility is a critical requirement.

Imo this is idealist view of the world 😅 . In reality users of the library will use bits and pieces of the library which are most interesting to them. And hack around the rest.

The only way around it is to make library api surface as small as possible and as type controlled as possible, so that core operations happens on library custom types, and all conversions to other types happens on caller side (or in separate helpers provider by the library in some helper packages)

[KonradStaniec]

just to understand how to interaction would look like api wise, it would be something like:

// First create Staking object
var stakingData =  new Staking(network, stakerInfo, params, finalityPkHex, stakingTimelock)
// then create staking tx when creating EOI
var stakingTxResult = stakingData.createStakingTransaction(stakingAmt, inputs, feeRate)
// before sending to BTC create psbt for signing
var psbt = stakingData.toStakingPsbt(stakingTxResult.transaction, inputs)

If this understanding is correct, then two things makes a bit nervous:

1. Transaction passed to toStakingPsbt, can be some other transaction i.e there is not protection agains passing wrong type of the transaction
2. both createStakingTransaction and toStakingPsbt takes inputs as parameter, and I assume this must be the same inputs, otherwise we will hit some weird signing errors.

What do you think about:

1. createStakingTransaction returning some new type called StakingTransactionResult, which will contain all necessary data to create Psbt (like utxo)
2. and toStakingPsbt accepting this StakingTransactionResult as parameter.

This would make it clear to the caller how to use the library at type system level.

(this comment probably applies to other types of psbts)

[jrwbabylonlab]

// before sending to BTC create psbt for signing var psbt = stakingData.toStakingPsbt(stakingTxResult.transaction, inputs)

No, this is a wrong statement. We don't use the transaction data output from FE when submitting to BTC. The transaction is a transactionHex which we receive from API (coming from BBN event).

Transaction passed to toStakingPsbt, can be some other transaction i.e there is not protection agains passing wrong type of the transaction

Could you help double check what else we could add in terms of validation for the stakingTx and unbondingTx that's being passed into the two methods? We can add those after testnet

@KonradStaniec u actually reminds me a security question. Staker now need to sign tx that coming from the backend service. What's the worst thing could happen if say API being hijacked and returned purposely made tx outs by hacker?

[KonradStaniec]

What's the worst thing could happen if say API being hijacked and returned purposely made tx outs by hacker?

what do you mean by hijacked ? Is this the case in which there is some kind of man in the middle attack in which front end is receiving bad data from backend ?

If this is possible then funds can be stolen i.e if front end receives staking transaction which points to staker utxos, but output in staking transaction point to attacker address.

Imo this should be impossible, as if this is possible then no amount of validation will help.

In my mind the chain of trust in out systems goes like:

• Backend trust Babylon node to which is connected, this means authenticated and encrypted connection (https) to Full Babylon node.
• FrontEnd trust Backend to which is connected, his means authenticated and encrypted connection (https) to Backend server
  so in practice it is as if Frontend would be connected to Full Babylon node with all its validation rules.

[vitsalis]

Two comments here:

1. The library should perform a full verification that this is a staking transaction with the required parameters.
2. Super important: The front-end dApp should not blindly trust that the API returns it a valid transaction to sign. If the API gets hijacked, we might be viewing a situation in which the transaction returned by the API is a "stealing funds" transaction and the user blindly signs it. The client side code should always perform the necessary verifications that this is indeed a staking transaction.

Note that this is different than the trust the client side has to the API for receiving parameters. These are parameters used to construct a staking transaction. Getting transactions from the API and signing them without additional verification is a big no-go.

[jrwbabylonlab]

True. But I think it worth us adding such validation by reconstructing the transaction and psbt from scratch to compare the output script/address shall match with what we would expect.

[jrwbabylonlab]

Note that i would not add those validation in this PR, they shall be handled at later stage and likely gonna be after testnet. But agree this is a must have for mainnet. right now, just don't have the bandwidth for it

[vitsalis]

Can you update the PR description with the list of things that should follow this PR and whether they will happen before testnet? It's super important to not lose track of these, as I'm starting to worry we are offloading a lot of things after the testnet.

[jrwbabylonlab]

Will do

as I'm starting to worry we are offloading a lot of things after the testnet.

Yep. Unfortunately with limited time, that's the only approach we can take without delaying the testnet.
It high priority in terms of security, but don't think it's high priority for testnet

[jrwbabylonlab]

I see this as a step in good direction for the library.

In general I think in final form, the library:

• should return most of the results in custom domain types when building transactions like: StakingTransaction, UnbodningTransaction etc. Those types do not need to be related to any BTC or psbt related types.
• it should provide helpers to convert our custom domain types into most popular types used in ecosystem i.e if caller want to sign there should be toPsbt function, if caller want to send tx to Babylon there should be toTransaction function etc

This would separate our domain logic (building staking scripts which are valid according to Babylon) from all the different custom formats caller can encounter.

Yep, this can definitely be done easily as part of a future PR. However, we still can’t avoid having a method for Tx -> PSBT, which is the core focus of this PR. When the frontend receives data from BBN, we get a transactionHex that is neither our own type nor something we can cast into our own type.

[KonradStaniec]

However, we still can’t avoid having a method for Tx -> PSBT, which is the core focus of this PR. When the frontend receives data from BBN, we get a transactionHex that is neither our own type nor something we can cast into our own type.

hmm but we know that the transaction we receive from Babylon is staking transaction, so we can go with chain:

txHex -> CustomStakingType -> PSBT

i.e whole conversion of types happens at the edges of the system. In the middle of the system we only operate on our custom domain types. When we want to operate with other systems we convert them again at the edges.

[jrwbabylonlab]

However, we still can’t avoid having a method for Tx -> PSBT, which is the core focus of this PR. When the frontend receives data from BBN, we get a transactionHex that is neither our own type nor something we can cast into our own type.

hmm but we know that the transaction we receive from Babylon is staking transaction, so we can go with chain:

txHex -> CustomStakingType -> PSBT

i.e whole conversion of types happens at the edges of the system. In the middle of the system we only operate on our custom domain types. When we want to operate with other systems we convert them again at the edges.

Ah, I understand now. I actually implemented this in my initial changes for the PR but later realized it might be over-engineering for the current needs (testnet launch).

That said, I completely agree that this approach would significantly improve code readability. I’ll definitely create a ticket to track this for future work—it essentially aligns with my first point in the PR description.

[vitsalis]

lgtm. Didn't check the test files.

[vitsalis]

Two comments here:

1. The library should perform a full verification that this is a staking transaction with the required parameters.
2. Super important: The front-end dApp should not blindly trust that the API returns it a valid transaction to sign. If the API gets hijacked, we might be viewing a situation in which the transaction returned by the API is a "stealing funds" transaction and the user blindly signs it. The client side code should always perform the necessary verifications that this is indeed a staking transaction.

Note that this is different than the trust the client side has to the API for receiving parameters. These are parameters used to construct a staking transaction. Getting transactions from the API and signing them without additional verification is a big no-go.

[KonradStaniec]

I agree with the direction of this pr. Ultimately we should go in direction of custom types in the core of the library, and do all type conversion at the edges (as described in comments - #44 (comment))

Though this can be done in subsequent pr's