[RFC][Tx Sort] Implement sorting of inputs #84

savil · 2018-07-24T03:26:51Z

Follows BIP69: https://github.com/bitcoin/bips/blob/master/bip-0069.mediawiki

Implements sorting of transaction inputs.

BIP says to use "reversed byte-order" for the prev_hash. I interpreted this as: little-endian.
TODO need to add tests

Re: improve sorting of TxOut's script_pubkey to use lexicographic ordering, and not length.
From the test-cases i've included it seems that the current code already does lexicographic ordering (and not length based). Am i missing something?

Follows BIP69: https://github.com/bitcoin/bips/blob/master/bip-0069.mediawiki 1. Implements sorting of transactoin inputs. - BIP says to use "reversed byte-order" for the `prev_hash`. I interpreted this as: little-endian. - TODO need to add tests 2. Re: improve sorting of TxOut's script_pubkey to use lexicographic ordering, and not length. From the test-cases i've included it seems that the current code already does lexicographic ordering (and not length based). Am i missing something?

TheBlueMatt · 2018-07-24T14:55:57Z

Cool, thanks. Can you add the test vectors from the BIP (and add some test vectors to the BIP that test variable-length scriptPubKeys)?

savil · 2018-07-24T19:53:27Z

@TheBlueMatt sure, yeah happy to, but I've been running into some issues.

For example, the given values in the BIP don't seem to serialize-deserialize cleanly. See the test output at the bottom of https://gist.github.com/savil/d6acec10aa5a8ea6698fbb19443d68cb. Not yet sure if that indicates a bug in the hex-string or in the rust-bitcoin's serialize/deserialize implementation.

Also, looks like rustc-serialize is now deprecated, so looking into using the hex library, https://users.rust-lang.org/t/deprecation-of-rustc-serialize/10485 but that one cannot deserialize the other hex-string in the BIP cleanly. https://gist.github.com/savil/dbe9963a263baba4d32de680c01e7c9e

TheBlueMatt · 2018-07-24T19:58:45Z

Weird, is it a 0-input transaction (rust-bitcoin/rust-bitcoin#104) cause then the BIP would need updating? Generally I've been using the hex_bytes from bitcoin::util::misc in tests, but I probably should have been using from_hex.

savil · 2018-07-24T20:26:55Z

@TheBlueMatt hmm, I need to learn BitcoinScript :p. Can you tell if they are 0-input from the info below?

41046a0765b5865641ce08dd39690aade26dfbf5511430ca428a3089261361cef170e3929a68aee3d8d4848b0c5111b0a37b82b86ad559fd2a745b44d8e8d9dfdc0ca
- gets printed as Script(OP_PUSHBYTES_4 6a0765b5 OP_XOR OP_PUSHNUM_6 OP_PUSHBYTES_65 <push past end>)
41044a656f065871a353f216ca26cef8dde2f03e8c16202d2e8ad769f02032cb86a5eb5e56842e92e19141d60a01928f8dd2c875a390f67c1f6c94cfc617c0ea45afac
- gets printed as Script(OP_PUSHBYTES_4 4a656f06 OP_PUSHNUM_8 OP_2ROT OP_MIN OP_PUSHNUM_3 OP_RETURN_242 OP_PUSHBYTES_22 ca26cef8dde2f03e8c16202d2e8ad769f02032cb86a5 OP_RETURN_235 OP_PUSHNUM_14 OP_PUSHNUM_6 OP_AND OP_PUSHBYTES_46 <push past end>)

EDIT: I made a copy-pasta error in the first hex-string above. Fixed it now.

TheBlueMatt · 2018-07-25T01:12:44Z

Ohoh, sorry, I misread. So your problem is you're trying to deserialize the script with a length byte, but the script in the BIP does not have its length byte included. You want to just convert the hex string to a Vec and do Script::from().

TheBlueMatt · 2018-07-25T01:13:51Z

Also note current patch uses spaces instead of tabs in the tests mod. We really need editor configs in these files....

savil · 2018-07-25T04:12:29Z

thank you. Didn't work for me, though. The code I have is:

// this line gives error: 
// panicked at 'called `Result::unwrap()` on an `Err` value: OddLength', 
// for hex_str equal to 
// "41044a656f065871a353f216ca26cef8dde2f03e8c16202d2e8ad769f02032cb86a5eb5e56842e92e19141d60a01928f8dd2c875a390f67c1f6c94cfc617c0ea45afaca""
let hex_script: Vec<u8> = decode(hex_str).unwrap();  

let script: Script = Script::from(hex_script);

(sorry, I incorrectly pointed out the errant line in my previous gist up top)

yuntai · 2018-07-25T08:01:48Z

@savil
You seem to have extra 'a' at the end of your hex_str.

savil · 2018-07-25T16:33:16Z

@yuntai much obliged, thanks!

TheBlueMatt

Probably needs a BIP/BOLT update here first either way.

TheBlueMatt · 2018-07-25T21:01:19Z

src/util/transaction_utils.rs

 			Ordering::Greater
 		} else {
 			Ordering::Equal
 		}
 	});
 }
+
+// TODO savil. Add tests.


Cool, so still need the tests added here.

savil · 2018-07-26T01:28:23Z

updated with BIP69 test cases.

and add some test vectors to the BIP that test variable-length scriptPubKeys)?

@TheBlueMatt why is variable-length the important property to focus on? I agree we need to augment BIP69 spec to have a test case where the output values are the same.

savil · 2018-07-26T01:29:35Z

Cargo.toml

@@ -20,6 +20,7 @@ bitcoin = "0.13"
 rust-crypto = "0.2"
 rand = "0.4"
 secp256k1 = "0.9"
+hex = "0.3.2"


I'll make this test-only

TheBlueMatt · 2018-07-26T15:30:46Z

I'm not actually sure if we ever call sort_outputs with different-length scripts, but if we do sort_outputs is clearly wrong - Rust's comparison will sort first by length, then by contents, whereas the BIP seems to (but doesn't actually explicitly say) that you should sort first by contents, then by length, as if by memcmp. Hence my comment that the BIP is underspecifed and needs to be fleshed out, with more test vectors added to it.

TheBlueMatt · 2018-07-26T15:31:30Z

But, given the BIP is generally a terrible idea for anything but Lightning, I'd really strongly prefer we update the BOLT to be specified, and not reference the BIP at all.

savil · 2018-07-26T15:36:54Z

src/util/transaction_utils.rs

+
+		let txout2 = TxOut {
+			value: 100,
+			script_pubkey: Builder::new().push_int(1).push_int(2).into_script()


I'm not actually sure if we ever call sort_outputs with different-length scripts,

@TheBlueMatt I'm confused. Does this test-case not exercise different-length scripts? if not, could you advise me on how to construct a different-length script?

Oh, indeed, sorry I missed that, somehow I concluded Rust's comparison did something different.

TheBlueMatt · 2018-07-26T15:49:13Z

Cargo.toml

@@ -27,3 +27,6 @@ gcc = "0.3"
 [dev-dependencies.bitcoin]
 version = "0.13"
 features = ["bitcoinconsensus"]
+
+[dev-dependencies]
+hex = "0.3.2"


I'm generally fine with this, but now we've got two ways of doing hex - using rust-bitcoin's utils one, and this. It probably makes sense to convert them all to the hex crate, cause using an exported rust-bitcoin util method always felt strange to me.

Also, not gonna block on this, but would be nice to fix in a followup PR.

ack, I'll look into it after I wrap up rust-bitcoin/rust-bitcoin#107

TheBlueMatt · 2018-07-26T15:54:06Z

src/util/transaction_utils.rs

+
+	use bitcoin::blockdata::script::{Script, Builder};
+	use bitcoin::blockdata::transaction::TxOut;
+    use bitcoin::util::hash::Sha256dHash;


There's a few tab/space things in the diff here - I always set my editor to auto-detect whether it should use tabs or spaces based on the file, and also highlight tabs and spaces differently so they're instantly visible.

crap. thought I'd fixed all of these. My apologies.

No big deal, I'm generally happy to clean up simple stuff like that post-merge (see 7f46025)

…ge-name

TheBlueMatt reviewed Jul 25, 2018

View reviewed changes

savil commented Jul 26, 2018

View reviewed changes

adding BIP69 test-cases

da09c4d

TheBlueMatt mentioned this pull request Jul 26, 2018

Do not reference the underspecified (and generally discouraged) BIP 69 lightning/bolts#456

Closed

savil commented Jul 26, 2018

View reviewed changes

TheBlueMatt reviewed Jul 26, 2018

View reviewed changes

TheBlueMatt merged commit fe9bb1d into lightningdevkit:master Jul 26, 2018

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Jul 26, 2018

Fix some tab-space errors in lightningdevkit#84

7f46025

savil deleted the sort_outputs branch July 27, 2018 23:39

This was referenced Jul 28, 2018

replace use of bitcoin::utils::misc::hex_bytes with hex::decode #95

Merged

cleanup TODOs in transaction_utils.rs #66

Closed

carlaKC pushed a commit to carlaKC/rust-lightning that referenced this pull request Aug 9, 2023

Merge pull request lightningdevkit#84 from tnull/2022-10-update-packa…

51bf3ce

…ge-name

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC][Tx Sort] Implement sorting of inputs #84

[RFC][Tx Sort] Implement sorting of inputs #84

savil commented Jul 24, 2018 •

edited

Loading

TheBlueMatt commented Jul 24, 2018

savil commented Jul 24, 2018

TheBlueMatt commented Jul 24, 2018

savil commented Jul 24, 2018 •

edited

Loading

TheBlueMatt commented Jul 25, 2018

TheBlueMatt commented Jul 25, 2018

savil commented Jul 25, 2018

yuntai commented Jul 25, 2018 •

edited

Loading

savil commented Jul 25, 2018

TheBlueMatt left a comment

TheBlueMatt Jul 25, 2018

savil commented Jul 26, 2018

savil Jul 26, 2018

TheBlueMatt commented Jul 26, 2018

TheBlueMatt commented Jul 26, 2018

savil Jul 26, 2018

TheBlueMatt Jul 26, 2018

TheBlueMatt Jul 26, 2018

TheBlueMatt Jul 26, 2018

savil Jul 26, 2018

TheBlueMatt Jul 26, 2018

savil Jul 26, 2018

TheBlueMatt Jul 26, 2018

[RFC][Tx Sort] Implement sorting of inputs #84

[RFC][Tx Sort] Implement sorting of inputs #84

Conversation

savil commented Jul 24, 2018 • edited Loading

TheBlueMatt commented Jul 24, 2018

savil commented Jul 24, 2018

TheBlueMatt commented Jul 24, 2018

savil commented Jul 24, 2018 • edited Loading

TheBlueMatt commented Jul 25, 2018

TheBlueMatt commented Jul 25, 2018

savil commented Jul 25, 2018

yuntai commented Jul 25, 2018 • edited Loading

savil commented Jul 25, 2018

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savil commented Jul 26, 2018

Choose a reason for hiding this comment

TheBlueMatt commented Jul 26, 2018

TheBlueMatt commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savil commented Jul 24, 2018 •

edited

Loading

savil commented Jul 24, 2018 •

edited

Loading

yuntai commented Jul 25, 2018 •

edited

Loading