Update bitcoin + ldk + secpzkp #197
Conversation
Tibo-lg
force-pushed
the
chore/update-bitcoin-dependency
branch
from
943b76a to
376a4e8
Compare
Tibo-lg
force-pushed
the
chore/update-bitcoin-dependency
branch
from
376a4e8 to
ad3e360
Compare
bitcoin-rpc-provider/src/lib.rs
Outdated
| fn u8slice_to_string(input: &[u8]) -> String { | ||
| use core::fmt::Write; | ||
| let mut ret = String::with_capacity(2 * input.len()); | ||
| for ch in input { | ||
| write!(ret, "{:02x}", ch).expect("writing to string"); | ||
| } | ||
| ret | ||
| } | ||
|
|
There was a problem hiding this comment.
all the rust-bitcoin projects are now using the hex-conservative crate, could be worth just using that here
dlc/src/util.rs
Outdated
| _ => { | ||
| let mut bytes = PushBytesBuf::new(); | ||
| bytes.extend_from_slice(redeem.as_bytes()).unwrap(); | ||
| ScriptBuf::new_witness_program(&WitnessProgram::new(WitnessVersion::V1, bytes).unwrap()) |
There was a problem hiding this comment.
witness v1 is taproot, this doesn't seem correct
There was a problem hiding this comment.
Thanks fixed (was copy pasted from my taproot branch...)
There was a problem hiding this comment.
LGTM beyond @benthecarman's comment.
I left a couple of suggestions, but they probably belong in a separate PR.
| address: x | ||
| .address | ||
| .as_ref() | ||
| .map(|x| x.clone().assume_checked()) |
There was a problem hiding this comment.
Should we use https://docs.rs/bitcoin/latest/bitcoin/address/struct.Address.html#method.require_network instead? It might be a bit annoying to adapt the code base, but consumers are probably only gonna use an instance of Manager, for example, for one particular Network at a time.
There was a problem hiding this comment.
We can and I thought of it. But then it'd be like asking the node who just sent you an address to also tell you its network. I guess it'd be a double check that the node is not doing something weird, but it doesn't feel very useful to me. I can add it though let me know if you think it's useful.
There was a problem hiding this comment.
I think it's low priority and I would only do this in a separate PR.
But I do think it can be useful in some spots. What if one side uses a testnet change address by mistake, when they're actually setting up a mainnet DLC?
There was a problem hiding this comment.
Which spots are you referring to? I think in this particular place, it wouldn't change anything. I think all the other places are in test code.
There was a problem hiding this comment.
You are right, this is the only place where we receive an Address in production code.
I assumed that there were more spots in the code base 😅
We can and I thought of it. But then it'd be like asking the node who just sent you an address to also tell you its network. I guess it'd be a double check that the node is not doing something weird, but it doesn't feel very useful to me.
Wouldn't it be more like making sure that the address returned by the node corresponds to the expected network? As in, you would receive the address and then check that its Network matches the one stored in BitcoinCoreProvider. This might prevent some weirdness/bugs if someone intends to run this component for mainnet, but they point to a testnet node instead.
There was a problem hiding this comment.
So IIUC what you're proposing is that the constructor takes a Network parameter? I think that's fine but I wonder if then it wouldn't be easier to just check within the constructor that the node corresponds to the expected network and then forget about it.
| /// | ||
| /// Taken from a previous version of ldk as it was refactored into something less practical to use | ||
| /// externally. | ||
| pub(crate) fn derive_public_key<T: secp256k1_zkp::Signing>( |
There was a problem hiding this comment.
Since the docs talk about this being just for generating a per_commitment_key, should we give it a related name?
There was a problem hiding this comment.
It's used to derive quite many things, update the docs.
dlc-manager/src/channel/utils.rs
Outdated
| /// Derives a per-commitment-transaction revocation public key from its constituent parts. This is | ||
| /// the public equivalend of derive_private_revocation_key - using only public keys to derive a | ||
| /// public key instead of private keys. |
There was a problem hiding this comment.
Typo.
| /// Derives a per-commitment-transaction revocation public key from its constituent parts. This is | |
| /// the public equivalend of derive_private_revocation_key - using only public keys to derive a | |
| /// public key instead of private keys. | |
| /// Derives a per-commitment-transaction revocation public key from its constituent parts. This is | |
| /// the public equivalent of derive_private_revocation_key - using only public keys to derive a | |
| /// public key instead of private keys. |
Tibo-lg
force-pushed
the
chore/update-bitcoin-dependency
branch
from
ad3e360 to
2525615
Compare
Tibo-lg
force-pushed
the
chore/update-bitcoin-dependency
branch
from
2525615 to
5823129
Compare
No description provided.