Txin sequence numbers

[atacann]

Description

Use correct sequence numbers for Txin's with timelocks, also update timelock durations to current design

Linked Issues

• Closes Fix txin sequence numbers #422

[ekrembal]

Let's also move these 2.5 week, 2 week constants to some constants file. We can do this in a seperate pr. I will create an issue

[ekrembal]

is this needed?

[atacann]

No, but it doesn't hurt because we are casting i64 to u16. I think as Efe said we should prob move these deposit related constants to some config struct (not read through a toml file so only editable with code changes). Stuff like num_kickoffs, timetxs, timeouts etc. It should be a struct instead of constants so that we can adjust stuff like num_timetxs to be small easily for tests so that they run fast. But config::default() would have normal deposit values.

[ekrembal]

@ceyhunsen had a pr to seperate the Config to parts, maybe that can be helpful, since we can seperate Bridge specific constants to a sub-category.

[mmtftr]

I'd prefer a constants module for things that'd change internal sighashes, #signatures. (basically anything that can cause errors in the deposit mechanism should not be configurable by the user)

Also, as @atacann said, we can create a test constants module that would provide very small defaults for fast tests.

[mmtftr]

If we're keeping this line, I think the following would be more readable (in fact, I think the behavior in your code may be incorrect as it doesn't check the lower bound):

u16::try_from(max_withdrawal_time_block_count).expect("Max withdrawal ...");

[ceyhunsen]

I think @ekrembal's mention #175 won't solve this issue. Because everything you put on BridgeConfig can be modified by the toml file, you can't block it.

Also, it should be noted that BridgeConfig::default() is there for testing; You may change the defaults if you think it will benefit most of the tests. Therefore we should block entity binaries from starting if no custom configuration files are specified.

[mmtftr]

In order of preference:

1. Newtype paradigm for better API, more rigid enforcement of contract:

pub struct TxInArgs {
  outpoint: OutPoint,
  height: Option<u16>,
}

impl From<OutPoint> for TxInArgs {
...
}

impl From<Vec<OutPoint>> for Vec<TxInArgs> {
...
}

impl From<Vec<(OutPoint, u16)>> for Vec<TxInArgs> {
...
}

// Single API with easy usage
// create_tx_ins(my_outpoint_vec.into())
// create_tx_ins(&my_tx_in_args_vec)
pub fn create_tx_ins(tx_in_args: impl AsRef<[TxInArgs]>) -> Vec<TxIn> {...}

// If you want to go one level higher:
// create_tx_ins(&my_outpoint_slice)
// Might cause too much codegen, so consider monomorphizing if going this route
pub fn create_tx_ins(tx_in_args: impl AsRef<[impl Into<TxInArgs>]>) -> Vec<TxIn> {...}

2. Use tuples to enforce that heights and utxos must have the same length:

pub fn create_tx_ins_with_sequence(tx_in_args: impl AsRef<[(OutPoint, Option<u16>)]>) -> Vec<TxIn> {...}

[mmtftr]

Feel free to skip this, as it's likely out of scope for this PR:

I'd prefer new module for utils like this, builder::transaction::utils. 1500 line file is not the best DX

[ekrembal]

Let's create a new issue for this, can you create one? @mmtftr

[atacann]

I changed create_tx_ins similar to what Efe said. TxInArgs is a wrapper around Vec because you can't impl for a Vec directly. Also I just move TxInArgs instead of AsRef because we never need the ref currently ( so we don't need to clone to create TxIn vec).