Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Return tx instead of psbt #44

Merged
merged 18 commits into from
Nov 25, 2024
Merged

Return tx instead of psbt #44

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
7d225f6
feat: return tx along with psbt
jrwbabylonlab Nov 19, 2024
6c1b56f
feat: reduce signing in eoi
jrwbabylonlab Nov 21, 2024
b0cdefa
add test for the stakingPsbt
jrwbabylonlab Nov 21, 2024
7e8869a
add test for the unbondingPsbt
jrwbabylonlab Nov 21, 2024
0858d15
add more tests
jrwbabylonlab Nov 21, 2024
b8d7ebd
remove unused code
jrwbabylonlab Nov 21, 2024
fa98387
set the jest back to run 10 times
jrwbabylonlab Nov 21, 2024
df6f3b8
update unbondingPsbt signature
jrwbabylonlab Nov 21, 2024
f451c77
bump to canary.3
jrwbabylonlab Nov 21, 2024
8197e0d
fix: bug with the txid being reverse order
jrwbabylonlab Nov 22, 2024
ff2f11c
fix: remove the covenant signer as part of the psbt construction
jrwbabylonlab Nov 22, 2024
c5be034
update the createWitness covenantSigs type
jrwbabylonlab Nov 22, 2024
5513b98
fix tests
jrwbabylonlab Nov 22, 2024
fa54029
update comments
jrwbabylonlab Nov 22, 2024
fec82d5
remove unused import
jrwbabylonlab Nov 22, 2024
f8f3753
chore: rename the createWithdrawEarlyUnbondedTransaction input parame…
jrwbabylonlab Nov 23, 2024
417b59f
review comment updates
jrwbabylonlab Nov 24, 2024
c8a578d
review comment updates 2
jrwbabylonlab Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@babylonlabs-io/btc-staking-ts",
"version": "0.4.0-canary.2",
"version": "0.4.0-canary.3",
"description": "Library exposing methods for the creation and consumption of Bitcoin transactions pertaining to Babylon's Bitcoin Staking protocol.",
"module": "dist/index.js",
"main": "dist/index.cjs",
Expand Down
137 changes: 110 additions & 27 deletions src/staking/index.ts
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file requires formatting

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { networks, Transaction } from "bitcoinjs-lib";
import { networks, Psbt, Transaction } from "bitcoinjs-lib";
import { StakingParams } from "../types/params";
import { UTXO } from "../types/UTXO";
import { StakingScriptData, StakingScripts } from "./stakingScript";
Expand All @@ -21,8 +21,9 @@ import {
validateStakingTimelock,
validateStakingTxInputData,
} from "../utils/staking";
import { PsbtTransactionResult } from "../types/transaction";
import { PsbtResult, TransactionResult } from "../types/transaction";
import { toBuffers } from "../utils/staking";
import { stakingPsbt, unbondingPsbt } from "./psbt";
export * from "./stakingScript";

export interface StakerInfo {
Expand Down Expand Up @@ -117,13 +118,15 @@ export class Staking {
* @param {UTXO[]} inputUTXOs - The UTXOs to use as inputs for the staking
* transaction.
* @param {number} feeRate - The fee rate for the transaction in satoshis per byte.
* @returns {PsbtTransactionResult} - An object containing the unsigned psbt and fee
* @returns {TransactionResult} - An object containing the unsigned transaction
* and fee
* @throws {StakingError} - If the transaction cannot be built
*/
public createStakingTransaction(
stakingAmountSat: number,
inputUTXOs: UTXO[],
feeRate: number,
): PsbtTransactionResult {
): TransactionResult {
validateStakingTxInputData(
stakingAmountSat,
this.stakingTimelock,
Expand All @@ -135,15 +138,28 @@ export class Staking {
const scripts = this.buildScripts();

try {
return stakingTransaction(
const { transaction, fee } = stakingTransaction(
scripts,
stakingAmountSat,
this.stakerInfo.address,
inputUTXOs,
this.network,
feeRate,
isTaproot(this.stakerInfo.address, this.network) ? Buffer.from(this.stakerInfo.publicKeyNoCoordHex, "hex") : undefined,
);
// Do a dry run of stakingPsbt to ensure the transaction can be converted to PSBT
jrwbabylonlab marked this conversation as resolved.
Show resolved Hide resolved
// with all the required properties before returning it
stakingPsbt(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@jrwbabylonlab jrwbabylonlab Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

transaction,
this.network,
inputUTXOs,
isTaproot(
this.stakerInfo.address, this.network
) ? Buffer.from(this.stakerInfo.publicKeyNoCoordHex, "hex") : undefined,
);
return {
transaction,
fee,
};
} catch (error: unknown) {
throw StakingError.fromUnknown(
error, StakingErrorCode.BUILD_TRANSACTION_FAILURE,
Expand All @@ -152,16 +168,40 @@ export class Staking {
}
};

/**
* Create a staking psbt based on the existing staking transaction.
*
* @param {Transaction} stakingTx - The staking transaction.
* @param {UTXO[]} inputUTXOs - The UTXOs to use as inputs for the staking
* transaction. The UTXOs that were used to create the staking transaction should
* be included in this array.
* @returns {Psbt} - The psbt.
*/
public toStakingPsbt(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

@jrwbabylonlab jrwbabylonlab Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

// 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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

stakingTx: Transaction,
inputUTXOs: UTXO[],
): Psbt {
return stakingPsbt(
stakingTx,
this.network,
inputUTXOs,
isTaproot(
this.stakerInfo.address, this.network
) ? Buffer.from(this.stakerInfo.publicKeyNoCoordHex, "hex") : undefined,
);
}

/**
* Create an unbonding transaction for staking.
*
* @param {Transaction} stakingTx - The staking transaction to unbond.
* @returns {PsbtTransactionResult} - An object containing the unsigned psbt and fee
* @returns {TransactionResult} - An object containing the unsigned transaction
* and fee
* @throws {StakingError} - If the transaction cannot be built
*/
public createUnbondingTransaction(
stakingTx: Transaction,
) : PsbtTransactionResult {
) : TransactionResult {
// Build scripts
const scripts = this.buildScripts();

Expand All @@ -173,14 +213,25 @@ export class Staking {
)
// Create the unbonding transaction
try {
const { psbt } = unbondingTransaction(
const { transaction } = unbondingTransaction(
scripts,
stakingTx,
this.params.unbondingFeeSat,
this.network,
stakingOutputIndex,
);
return { psbt, fee: this.params.unbondingFeeSat };
// Do a dry run of unbondingPsbt to ensure the transaction can be converted to PSBT
// with all the required properties before returning it
unbondingPsbt(
scripts,
transaction,
stakingTx,
this.network,
);
return {
transaction,
fee: this.params.unbondingFeeSat,
};
} catch (error) {
throw StakingError.fromUnknown(
error, StakingErrorCode.BUILD_TRANSACTION_FAILURE,
Expand All @@ -190,25 +241,51 @@ export class Staking {
}

/**
* Create a withdrawal transaction that spends an unbonding transaction for observable staking.
* Create an unbonding psbt based on the existing unbonding transaction and
* staking transaction.
*
* @param {Transaction} unbondingTx - The unbonding transaction to withdraw from.
* @param {number} feeRate - The fee rate for the transaction in satoshis per byte.
* @returns {PsbtTransactionResult} - An object containing the unsigned psbt and fee
* @throws {StakingError} - If the delegation is invalid or the transaction cannot be built
* @param {Transaction} unbondingTx - The unbonding transaction.
* @param {Transaction} stakingTx - The staking transaction.
*
* @returns {Psbt} - The psbt.
*/
public createWithdrawEarlyUnbondedTransaction (
public toUnbondingPsbt(
unbondingTx: Transaction,
stakingTx: Transaction,
): Psbt {
return unbondingPsbt(
this.buildScripts(),
unbondingTx,
stakingTx,
this.network,
);
}

/**
* Creates a withdrawal transaction that spends from an unbonding or slashing
* transaction. The timelock on the input transaction must have expired before
* this withdrawal can be valid.
*
* @param {Transaction} earlyUnbondedTx - The unbonding or slashing
* transaction to withdraw from
* @param {number} feeRate - Fee rate in satoshis per byte for the withdrawal
* transaction
* @returns {PsbtResult} - Contains the unsigned PSBT and fee amount
* @throws {StakingError} - If the input transaction is invalid or withdrawal
* transaction cannot be built
*/
public createWithdrawEarlyUnbondedTransaction (
earlyUnbondedTx: Transaction,
feeRate: number,
): PsbtTransactionResult {
): PsbtResult {
// Build scripts
const scripts = this.buildScripts();

// Create the withdraw early unbonded transaction
try {
return withdrawEarlyUnbondedTransaction(
scripts,
unbondingTx,
earlyUnbondedTx,
this.stakerInfo.address,
this.network,
feeRate,
Expand All @@ -227,13 +304,13 @@ export class Staking {
*
* @param {Transaction} stakingTx - The staking transaction to withdraw from.
* @param {number} feeRate - The fee rate for the transaction in satoshis per byte.
* @returns {PsbtTransactionResult} - An object containing the unsigned psbt and fee
* @returns {PsbtResult} - An object containing the unsigned psbt and fee
* @throws {StakingError} - If the delegation is invalid or the transaction cannot be built
*/
public createWithdrawTimelockUnbondedTransaction(
public createWithdrawStakingExpiredTransaction(
stakingTx: Transaction,
feeRate: number,
): PsbtTransactionResult {
): PsbtResult {
// Build scripts
const scripts = this.buildScripts();

Expand Down Expand Up @@ -266,12 +343,12 @@ export class Staking {
* Create a slashing transaction spending from the staking output.
*
* @param {Transaction} stakingTx - The staking transaction to slash.
* @returns {PsbtTransactionResult} - An object containing the unsigned psbt and fee
* @returns {PsbtResult} - An object containing the unsigned psbt and fee
* @throws {StakingError} - If the delegation is invalid or the transaction cannot be built
*/
public createStakingOutputSlashingTransaction(
stakingTx: Transaction,
) : PsbtTransactionResult {
) : PsbtResult {
if (!this.params.slashing) {
throw new StakingError(
StakingErrorCode.INVALID_PARAMS,
Expand All @@ -292,7 +369,10 @@ export class Staking {
this.params.slashing.minSlashingTxFeeSat,
this.network,
);
return { psbt, fee: this.params.slashing.minSlashingTxFeeSat };
return {
psbt,
fee: this.params.slashing.minSlashingTxFeeSat,
};
} catch (error) {
throw StakingError.fromUnknown(
error, StakingErrorCode.BUILD_TRANSACTION_FAILURE,
Expand All @@ -305,12 +385,12 @@ export class Staking {
* Create a slashing transaction for an unbonding output.
*
* @param {Transaction} unbondingTx - The unbonding transaction to slash.
* @returns {PsbtTransactionResult} - An object containing the unsigned psbt and fee
* @returns {PsbtResult} - An object containing the unsigned psbt and fee
* @throws {StakingError} - If the delegation is invalid or the transaction cannot be built
*/
public createUnbondingOutputSlashingTransaction(
unbondingTx: Transaction,
): PsbtTransactionResult {
): PsbtResult {
if (!this.params.slashing) {
throw new StakingError(
StakingErrorCode.INVALID_PARAMS,
Expand All @@ -330,7 +410,10 @@ export class Staking {
this.params.slashing.minSlashingTxFeeSat,
this.network,
);
return { psbt, fee: this.params.slashing.minSlashingTxFeeSat };
return {
psbt,
fee: this.params.slashing.minSlashingTxFeeSat,
};
} catch (error) {
throw StakingError.fromUnknown(
error, StakingErrorCode.BUILD_TRANSACTION_FAILURE,
Expand Down
29 changes: 22 additions & 7 deletions src/staking/observable/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@ import { StakingError, StakingErrorCode } from "../../error";
import { stakingTransaction } from "../transactions";
import { isTaproot } from "../../utils/btc";
import { toBuffers, validateStakingTxInputData } from "../../utils/staking";
import { PsbtTransactionResult } from "../../types/transaction";
import { TransactionResult } from "../../types/transaction";
import { ObservableStakingScriptData, ObservableStakingScripts } from "./observableStakingScript";
import { StakerInfo, Staking } from "..";
import { networks } from "bitcoinjs-lib";
import { stakingPsbt } from "../psbt";
export * from "./observableStakingScript";

/**
Expand Down Expand Up @@ -104,13 +105,14 @@ export class ObservableStaking extends Staking {
* @param {UTXO[]} inputUTXOs - The UTXOs to use as inputs for the staking
* transaction.
* @param {number} feeRate - The fee rate for the transaction in satoshis per byte.
* @returns {PsbtTransactionResult} - An object containing the unsigned psbt and fee
* @returns {TransactionResult} - An object containing the unsigned transaction,
* and fee
*/
public createStakingTransaction(
stakingAmountSat: number,
inputUTXOs: UTXO[],
feeRate: number,
): PsbtTransactionResult{
): TransactionResult {
validateStakingTxInputData(
stakingAmountSat,
this.stakingTimelock,
Expand All @@ -123,25 +125,38 @@ export class ObservableStaking extends Staking {

// Create the staking transaction
try {
return stakingTransaction(
const { transaction, fee } = stakingTransaction(
scripts,
stakingAmountSat,
this.stakerInfo.address,
inputUTXOs,
this.network,
feeRate,
isTaproot(this.stakerInfo.address, this.network) ? Buffer.from(this.stakerInfo.publicKeyNoCoordHex, "hex") : undefined,
// `lockHeight` is exclusive of the provided value.
// For example, if a Bitcoin height of X is provided,
// the transaction will be included starting from height X+1.
// https://learnmeabitcoin.com/technical/transaction/locktime/
this.params.activationHeight - 1,
);

stakingPsbt(
transaction,
this.network,
inputUTXOs,
isTaproot(
this.stakerInfo.address, this.network
) ? Buffer.from(this.stakerInfo.publicKeyNoCoordHex, "hex") : undefined,
);

return {
transaction,
fee,
};
} catch (error: unknown) {
throw StakingError.fromUnknown(
error, StakingErrorCode.BUILD_TRANSACTION_FAILURE,
"Cannot build unsigned staking transaction",
);
}
};
}
}
}
Loading