cleanup TODOs in transaction_utils.rs

[savil]

I was looking into doing this as an easy starter task, but had some questions. https://github.com/rust-bitcoin/rust-lightning/blob/master/src/util/transaction_utils.rs

1. re: the TODO to make static and put in utils (+ sort inputs).

• not sure what "make static" would mean for a standalone function? I'm new to rust so probably overlooking some concept.
• it seems to already be in a utils file, and is called as transaction_utils::sort_outputs. Is there anything else to do (except implement sort_inputs)?

2. re: the TODO for ordering of scripts shouldn't be len-based.

• If not len-based, then what should the ordering be based on?
• I can poke around at other implementations to see what they do, but figured I'd ask first.

[savil]

cc @TheBlueMatt tagging you in case you didn't get a notification about this issue, and questions I had.

[TheBlueMatt]

Its already been moved to utils/made a standable function (it was previously in Channel), but it may need a second function to sort inputs (I think most uses are only one input, but maybe I'm wrong). As for the sort order, it should match https://github.com/lightningnetwork/lightning-rfc/blob/master/03-transactions.md#transaction-input-and-output-ordering which appears to not reference length and only sort based on the data itself, but BIP 69 is underspecified so its unclear.

[TheBlueMatt]

Honestly probably best to rewrite BOLT 3 to not reference BIP 69 since the BIP is underspecified and just write what is intended there.

[savil]

ah, cool, should've checked the BOLTs. It says "the respective output scriptPubKeys (as a byte-array) will be compared lexicographically, in ascending order". Looking at the btcd implementation (https://git.io/fNCx8), they use Bytes.compare, and I can find the rust-equivalent of that for now.

I'm not convinced its better for BOLT 3 to have its own spec of tx ordering. Then, the bitcoin ecosystem will have to deal with both BIP 69 and the BOLT3 versions, which could diverge over time. Feels better to add clarity to BIP69. At the least, provide a test-case where tx.amount are equal but the tx.scriptPubKeys are different.

[TheBlueMatt]

Generally, BIP69 is a bad idea. In any case where it's possible, input/output ordering should be randomized, not deterministic. Lightning can't do that, so needs something specified, but BIP 69 isn't clear, so just working around it seems fine to me.

…

On July 20, 2018 7:22:32 PM UTC, savil ***@***.***> wrote: ah, cool, should've checked the BOLTs. It says "the respective output scriptPubKeys (as a byte-array) will be compared lexicographically, in ascending order". Looking at the btcd implementation (https://git.io/fNCx8), they use `Bytes.compare`, and I can find the rust-equivalent of that for now. I'm not convinced its better for BOLT 3 to have its own spec of tx ordering. Then, the bitcoin ecosystem will have to deal with both BIP 69 and the BOLT3 versions, which could diverge over time. Feels better to add clarity to BIP69. At the least, provide a test-case where tx.amount are equal but the tx.scriptPubKeys are different. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #66 (comment)

[savil]

fixed in #84 and #95