--- Day changed Fri Aug 24 2018 00:09 < andytoshi> BlueMatt: should Vec::consensus_decode be rejecting nonminimal length bytes? 00:09 < andytoshi> looks like Core always does 00:24 < andytoshi> also, wtf, VarInt double-converts from LE ... so on a big-endian system it won't work. sigh. would be cool if we could find such a system to fuzz on 00:54 < andytoshi> https://github.com/rust-bitcoin/rust-bitcoin/pull/151 fixes all this 01:46 -!- Netsplit *.net <-> *.split quits: dongcarl 01:47 -!- Netsplit over, joins: dongcarl 02:32 -!- Netsplit *.net <-> *.split quits: BlueMatt, treyzania, andytoshi 02:32 -!- Netsplit over, joins: treyzania 02:32 -!- andytoshi [~apoelstra@96.53.77.134] has joined #rust-bitcoin 02:35 -!- BlueMatt [~BlueMatt@mail.bluematt.me] has joined #rust-bitcoin 02:38 -!- andytoshi [~apoelstra@96.53.77.134] has quit [Changing host] 02:38 -!- andytoshi [~apoelstra@unaffiliated/andytoshi] has joined #rust-bitcoin 02:40 -!- BlueMatt [~BlueMatt@mail.bluematt.me] has quit [Quit: ZNC - http://znc.in] 03:06 -!- Netsplit *.net <-> *.split quits: andytoshi, treyzania 03:06 -!- Netsplit *.net <-> *.split quits: savil 03:06 -!- Netsplit *.net <-> *.split quits: sgeisler 03:06 -!- Netsplit *.net <-> *.split quits: nickler 03:07 -!- Netsplit over, joins: andytoshi, treyzania, savil, nickler, sgeisler 03:43 -!- BlueMatt [~BlueMatt@unaffiliated/bluematt] has joined #rust-bitcoin 03:48 -!- BlueMatt [~BlueMatt@unaffiliated/bluematt] has quit [Quit: Quit] 03:50 -!- BlueMatt [~BlueMatt@unaffiliated/bluematt] has joined #rust-bitcoin 17:53 < BlueMatt> andytoshi: you notice travis failed on your serialization pr? 17:55 < BlueMatt> nvm, looked like a nonsense result 17:55 < BlueMatt> cargo: Command not found 17:56 < BlueMatt> carto: Command not found 18:21 < BlueMatt> andytoshi: actually, regarding the serialization changes, isnt witness-encoding the only way to reliably encode a 0-input transaction, even if it has no witnesses? 18:21 < BlueMatt> that seems bad 18:30 < BlueMatt> andytoshi: see -core-dev 18:31 < andytoshi> grr, somehow i was not in -core-dev 18:31 < BlueMatt> ah, guess you missed it 18:31 < BlueMatt> see logs 18:31 < andytoshi> reading 18:31 < andytoshi> i'm really confused as to how i dropped out of the channel though.. 18:32 < andytoshi> ok, cool, i'll make an exception for 0-input transactions 18:33 < andytoshi> (we fixed this in elements because there we have a magic hardforking wand..) 18:33 < BlueMatt> and probably document that 0-input txn MUST be encoded with witnesses 18:33 < BlueMatt> well, "with witnesses" 18:34 < andytoshi> i can find somewhere to document that .. but i'll just make the `ConsensusEncodable` impl of `Transaction` do the right thing.. 18:35 < BlueMatt> cool 18:35 < BlueMatt> that probably needs a major version bump, then, though 18:36 < BlueMatt> at least the encode half 18:36 < BlueMatt> cause it'll spit out super surprising stuff for non-segwit peopl 18:36 < BlueMatt> e 18:36 < andytoshi> hmm 18:37 < andytoshi> ok i guess 18:37 < BlueMatt> can do that in a separate pr 18:37 < andytoshi> yeah, i'd prefer a separate PR then 18:37 < andytoshi> does PSBT actually avoid the need for input-less transactions? 18:38 < BlueMatt> tbh i havent looked much at psbt, but its billed as, in part, a fix to precisely this issue 18:38 < BlueMatt> so i assume so 18:43 < andytoshi> every PSBT includes an unsigned transaction in it 18:44 < BlueMatt> well I assume it fixes this somehow 18:44 < andytoshi> there is a "Creator" role who is supposed to add all the inputs 18:44 < BlueMatt> if not then its broken as fuck and doesnt fix *the* issue its supposed to 18:44 < BlueMatt> and needs to go back to the drawing board 18:44 < andytoshi> the issue it's supposed to fix is that there is no standard for doing multisig transactions 18:44 < andytoshi> but i'll ask achow on core-dev 18:45 < andytoshi> you can't tell a hw wallet what bip32 paths to use, etc., in any standard way 18:45 < BlueMatt> its also supposed to fix 0-input segwit-introduced ambiguity 18:45 < BlueMatt> since thats been a major issue 18:46 < BlueMatt> anyway, tell me when you've updated the reject-all-non-0-input-no-witness-extended-serialization pr 18:49 < andytoshi> will do 18:49 -!- TamasBlummer [~Thunderbi@p200300E38F0A0244AD7598278C15E281.dip0.t-ipconnect.de] has joined #rust-bitcoin 18:50 < andytoshi> sipa reminds me that the only tx in a PSBT is the -unsigned- tx (until it's finished) and it's required to be non-witness serialized 18:50 < andytoshi> so there is no ambiguity 18:51 < BlueMatt> well then we need a new fucking deserialization mode 18:51 < BlueMatt> cause thats a non-witness-encoded 0-input tx 18:51 < BlueMatt> which our deserializer will reject 18:51 < BlueMatt> thats a fucking stupid design 18:51 < andytoshi> sigh 18:53 < andytoshi> ok, so we're going to need to add some flags to ConsensusDecoder and ConsensusEncoder ... which i'd like to do anyway so that we can have a hashing mode 18:53 < andytoshi> and stop having `BitcoinHash` being separately implemented everywhere, that's really annoying 18:54 < BlueMatt> god damn it 18:54 < BlueMatt> this is a fucking dumb design 18:58 < BlueMatt> man, who the fuck reviewed this shit 18:59 < andytoshi> basically nobody until 3-4 months ago 18:59 < andytoshi> and then people doing multisig wallets 18:59 < andytoshi> which do not require 0-input transactions 18:59 < andytoshi> they require a sensible way to communicate redeem scripts, bip32 paths, etc 18:59 < andytoshi> and signatures .. without requiring every signer be able to parse scriptsigs or witnesses or produce them 19:00 < BlueMatt> anyway, as for our handling of it, can we not add flags to ConsensusDecoder and just have the ability to call the fn direction on Transaction? 19:00 < BlueMatt> and ConsensusEncode/Decode does the right thing for non-psbt 19:00 < BlueMatt> and psbt has to call the function manually 19:00 < andytoshi> we could .. but i want to add flags anyway for hashing 19:01 < BlueMatt> hashing? 19:01 < andytoshi> encoding into a sha256d engine 19:01 < andytoshi> right now i basically duplicate the ConsensusEncodable logic in impls of BitcoinHash, which is annoying 19:02 < andytoshi> some things are encoded differently depending whether they're being encoded into a hash or not (core also has this flag) 19:02 < andytoshi> though i can't think of any examples except blocks which simply pass through to their header when their hash is requested.. 19:02 < BlueMatt> what is ecndoed differently? 19:03 < andytoshi> well, in Elements blockheaders don't hash over the block signatures 19:03 < andytoshi> in Bitcoin the hash-serialization of a Block is its header and nothing else 19:03 < andytoshi> lemme check .. i'm sure there are a couple other examples 19:04 < andytoshi> hmm, no, in Bitcoin that's the only one .. never mind 19:04 < andytoshi> i guess there's no pressing need for a hash flag then 19:15 < andytoshi> BlueMatt: oh, actually our PSBT parser already has weird deserialization issues that require it use a separate trait from ConsensusDecodeable .. so the current PSBT PR has the infrastructure to support this 19:16 < BlueMatt> ugh 19:16 < andytoshi> (specifically, transactions are length-prefixed but vectors are not ... because vectors already have their own length prefix) 19:16 < BlueMatt> huh? 19:16 < andytoshi> s/vectors/scripts/ 19:16 < BlueMatt> wait, huh? 19:16 < andytoshi> in PSBT a transaction is prefixed by a VarInt indicating its length 19:16 < andytoshi> so you have to first deserialize as a Vec, then deserialize _that_ as a tx 19:17 < BlueMatt> why does that need a new trait? 19:17 < andytoshi> but to deserialize a script, you have to first deserialize to a Vec, then directly cast it to a Script 19:17 < BlueMatt> cant you just pull out a slice and the deserialize from the slice? 19:17 < andytoshi> yes, but that would do the wrong thing for scripts 19:18 < BlueMatt> ugh, now I'm confused 19:18 < andytoshi> if you want to deserialize a Transaction from a PSBT container, you have to use Vec::consensus_decode and then Transaction::consensus_decode 19:19 < andytoshi> if you want to deserialize a Script from a PSBT container, you have to do Script::consensus_decode 19:19 < andytoshi> the existing ConsensusDecode trait cannot describe this difference 19:19 < BlueMatt> wait, but dont you know what you're about to decode? 19:20 < andytoshi> for fields you understand, yes 19:20 < BlueMatt> and cant you skip the vec step if you just do a VarInt::consensus_decode, take slice, Transaction::consensus_decode(Thinggy(slice)) 19:20 < BlueMatt> ? 19:20 < andytoshi> that's very annoying to do in Rust 19:20 < andytoshi> but i'd have to reread the psbt PR to see why exactly 19:20 < BlueMatt> wait, huh? 19:21 < BlueMatt> why is that annoying to do? 19:21 < BlueMatt> adding a ton of additional deserializtion traits is probably more annoying, no? 19:23 < BlueMatt> anyway, guess we need a second decoder anyway for psbt 19:23 < andytoshi> the reason is that there is a macro `impl_psbt_insert_pair!` which defines the de/serialization code for a given key/value pair 19:23 < BlueMatt> cause it has to always do non-witness 19:23 < BlueMatt> vs non-psbt has to reject 0-input non-witness :( 19:23 < andytoshi> and if there is special-case code for every individual type the macro is harder 19:24 < BlueMatt> arent there, like....two things you ever serialize? 19:24 < BlueMatt> anyway, whatever, I give up 19:24 < BlueMatt> I'm just gonna never use psbt 19:24 < andytoshi> ? 19:24 < andytoshi> what do you mean "two things" 19:25 < BlueMatt> how much does psbt ever serialize that isnt either a) a transaction or b) a signature in a vec? 19:25 < andytoshi> scirpts, public keys, transactions, bip32 paths, bip32 fingerprints, and unknown (Vec) 19:25 < andytoshi> also SigHashType but IIRC that's not in the bip, it sounds like something carl did to make his life easier 19:26 < BlueMatt> ugh, whatever 19:26 < andytoshi> oh, no sighashtype is actually a separate field .. then maybe the sig is a normal DER sig instead of the annoying bitcoin-specific format .. would have to reread the bip 19:27 < andytoshi> this is all data that multisig signers need to be able to sign a transaction 19:27 < andytoshi> oh, also TxOut (for segwit inptu references) or Transaction (for non-segwit input references) 19:28 < BlueMatt> hmm? I was just told there was nothing that was aware of segwit in psbt 19:28 < andytoshi> nothing is aware of segwit encodings 19:28 < andytoshi> but signers know how to make segwit sighashes 19:28 < BlueMatt> oh, you mean for fee-calc 19:28 < andytoshi> yeah 19:29 < andytoshi> a PSBT processor that doesn't know anything at all about segwit will DTRT .. just pass around the "segwit utxo" field as raw bytes 19:29 < andytoshi> s/processor/Combiner/ 19:29 < BlueMatt> yea 19:34 < BlueMatt> anyway, back to my earlier point, sounds like psbt needs to have some way to always decode a tx as non-witness, which I dont think we want to do in consensus_decode? 19:34 < andytoshi> yeah 19:35 < andytoshi> i think it's fine if consensus_decode just uses the always-add-witness serialization, and psbt will have its own serialization 19:35 < BlueMatt> hmmm? we dont need to duplicate all the code 19:36 < andytoshi> it's literally 4 LOC to do the psbt deserialization 19:36 < BlueMatt> ok, fair 19:37 < andytoshi> self.version.consensus_encode(&mut enc).unwrap(); 19:37 < andytoshi> self.input.consensus_encode(&mut enc).unwrap(); 19:37 < andytoshi> self.output.consensus_encode(&mut enc).unwrap(); 19:37 < andytoshi> self.lock_time.consensus_encode(&mut enc).unwrap(); 19:37 < andytoshi> (not quite those, but i happened to have them for the BitcoinHash impl of Transaction) 19:37 < BlueMatt> s/encode/decode/ :p 19:37 < andytoshi> yeah :P 19:37 < BlueMatt> yea, i was thinking have to redo it for the inputs and outputs and such too 19:37 < BlueMatt> but, obviously not 19:44 < andytoshi> am i crazy or is `0100000000010100e1f505000000001976a9140389035a9225b3839e2bbf32d826a1e222031fd888ac00000000` a 0-in-1-out tx in the encoding we want? 19:44 < andytoshi> core rejects it and my code doesn't like it 19:45 < andytoshi> oh no i'm wrong 19:45 < andytoshi> i have to do 00, then 01, then 00 to indicate "no inputs" 19:46 < andytoshi> let me just run the fuzzer and i'll update the pr 19:50 < BlueMatt> andytoshi: wait, I thought you were gonna split the pr 19:51 < BlueMatt> 1 pr for a .1 only rejecting the non-witness non-0-input extended-serialization in deserialization 19:51 < BlueMatt> one for a new version bump that changes the serializer 19:51 < andytoshi> BlueMatt: oh, yeah, i can do that 19:51 < andytoshi> one sec .. luckily the difference is exactly one commit 19:52 < BlueMatt> thats what I figured :p 19:56 < BlueMatt> andytoshi: hmm? you still need to update #151 to *not* reject 0-input segwit-encoded txn 19:56 < andytoshi> oh i see 19:58 < andytoshi> fixed 20:01 < BlueMatt> andytoshi: why not reject non-minimal VarInts on the VarInt type? 20:01 < BlueMatt> why create a new type? there is ~0 excuse for non-minimal VarInts anywhere 20:02 < BlueMatt> also, why limit deserialize_script to 100 bytes? 20:02 < BlueMatt> did the re-serialization cause OOM's for you? 20:03 < andytoshi> BlueMatt: oops, i shouldn't have committed that 20:03 < andytoshi> no, i just wanted it to find smaller test cases 20:03 < andytoshi> cool, i can remove the new type if there is no reason for non-minimal varints 20:03 < BlueMatt> yea, I mean its *technically* breaking, but honestly I dont think its worth worrying about 20:03 < andytoshi> agreed 20:04 < BlueMatt> I'm not aware of anything that generates them and bitcoin core has rejected them on-the-wire forever 20:04 < andytoshi> i did a new type because i just didn't know if there was anything 20:04 < BlueMatt> so if anyone had been using them they've never passed their transaction to bitcoin core in any way 20:04 < andytoshi> right, yeah, i think since like 2010 or something 20:04 < andytoshi> it's not even called "minimal", it's called "compact", it's such an old thing.. 20:06 < TamasBlummer> even if rejected on wire. are you sure none minimal varint in a transaction mined into a block was rejected? 20:08 < BlueMatt> there is no such thing as the encoding in a block, blocks have always been re-serialized before being relayed, so any non-minimal encodings would have been "removed" 20:08 < BlueMatt> since satoshi-era 20:13 < TamasBlummer> if a node receives a block with some not minimal varint in a transaction script. does it parse and re-serialize all scripts while checking eg. merkle root? 20:14 < BlueMatt> varints arent "in" scripts, thats separate, but the varint describing the script length, yes, it *used* to 20:14 < BlueMatt> now it just rejects it when deserializing 20:14 < BlueMatt> it used to in block creation/acceptance/etc 20:14 < BlueMatt> not in tx relay, but thats a quirk, anything internal it used to 20:14 < TamasBlummer> i meant element lengths in scripts 20:15 < TamasBlummer> you are right they are not varints, just the total length 20:16 < BlueMatt> yea, the element lengths in scripts are allowed to be non-minimal, and are not re-encoded 20:16 < BlueMatt> they are, however, non-standard 20:17 < andytoshi> they are also not VarInts 20:17 < TamasBlummer> yes, they are explicit push_n opcodes 20:20 < BlueMatt> andytoshi: other than the test, 151 looks good 20:22 < andytoshi> ughh ok 20:22 < andytoshi> it is a huge pain to check error types 20:23 < andytoshi> because io::Error does not impl Eq or Debug 20:23 < BlueMatt> hmm? its not an io::Error? 20:24 < andytoshi> it contains one as a variant 20:24 < BlueMatt> just do an if let ... = err {} else { panic!() } 20:24 < andytoshi> ok :/ 20:26 -!- TamasBlummer [~Thunderbi@p200300E38F0A0244AD7598278C15E281.dip0.t-ipconnect.de] has quit [Quit: TamasBlummer] 20:32 < andytoshi> fixed 21:33 -!- andytoshi [~apoelstra@unaffiliated/andytoshi] has quit [Ping timeout: 272 seconds] 22:23 -!- andytoshi [~apoelstra@wpsoftware.net] has joined #rust-bitcoin 22:23 -!- andytoshi [~apoelstra@wpsoftware.net] has quit [Changing host] 22:23 -!- andytoshi [~apoelstra@unaffiliated/andytoshi] has joined #rust-bitcoin