fundrawtransaction can look like extended tx format.

[rustyrussell]

Seems to cause a misparse, since you're supposed to be able to hand a 0-input tx here. Seems like a new parameter to fundrawtransaction may be needed, to a hack to try parsing both ways...

[sipa]

I discovered this a few days ago as well. There is an easy solution: use
the extended format, but without witnesses.

So:

• u32 nVersion
• u8 marker = 0
• u8 flags = 0
• varint nTxins = 0
• varint nTxouts = ...
• CTxout txouts[nTxouts]
• nLocktime

Pieter

[rustyrussell]

Breaks backwards compatibility.

[rustyrussell]

Something like rustyrussell@fa68b4a ?

[sipa]

I'm aware, but is there a need for compatibility? Fundrawtransaction only deals with temporary data produced by createrawtransaction, not with valid transactions. I really want to avoid introducing extra flags or guessing.

[btcdrak]

I agree with @sipa. Note that fundrawtransactions was only just released with 0.12.0 just 6 weeks ago so it cannot be in widespread use. I think we should follow the least complicated path here. In this case, the disruption, if any, is minimal and we can put a note in the upcoming 0.12.1 release that the fundrawtransaction API will change in 0.12.2.

[sipa]

@btcdrak Technically, it is createrawtransaction that changed to produce extended format transactions when there are 0 inputs in the result. However, the only reason (that I know of) for using createrawtransaction in that way, is in order to pass the result to fundrawtransaction.

[rustyrussell]

Pieter Wuille [email protected] writes:

@btcdrak Technically, it is createrawtransaction that changed to produce
extended format transactions when there are 0 inputs in the result.
However, the only reason (that I know of) for using createrawtransaction in
that way, is in order to pass the result to fundrawtransaction.

Well, I was using fundrawtransaction without createrawtransaction
(createrawtransaction seems pretty useless to me).

But breaking createrawtransaction is pretty bad too.

Presumably these were put in place because people want to split
sendtoaddress into multiple steps (createrawtransaction,
fundrawtransaction, signrawtransaction, sendrawtransaction), implying
they're interpreting and modifying the tx somewhere along the way?

Pinging @TheBlueMatt who added these AFAICT...

[sipa]

Right, but the only reason you'd ever create a raw transaction with no inputs is because you plan to call fundrawtransaction. Given that fundrawtransaction is very new, I really prefer breaking it now rather than adding more clutter to the RPCs.

[NicolasDorier]

I hit the problem in nbitcoin as l often need to serialize transaction without input. The way I fixed that is by using a bit in nversion with special meaning only if the transaction has no input. (I set and unset this bit silently during serialization and deserialisation)

See readwrite method in https://github.com/MetacoSA/NBitcoin/blob/master/NBitcoin/Transaction.cs

not sure if it helps.

[rustyrussell]

Nicolas Dorier [email protected] writes:

I hit the problem in nbitcoin as l often need to serialize transaction without input. The way I fixed that is by using a bit in nversion with special meaning only if the transaction has no input. (I set and unset this bit silently during serialization and deserialisation)

See readwrite method in https://github.com/MetacoSA/NBitcoin/blob/master/NBitcoin/Transaction.cs

not sure if it helps.

I think we're better off trying the old-style deserialization if
new-style fails.

Also, I think the createrawtransaction needs a flag to say "create an
extended transaction", which is false by default.

[sipa]

I really do not want to introduce guessing (creates unclear expectations) or extra flags (a terrible technical debt that we'd very much regret in a post-segwit world).

But I think there is a better solution: fundrawtransaction does not need to support new-style parsing, as it deals with transactions that don't have signatures yet, and thus, no witnesses.

So I think I can revert the change to use new-style serialization for transactions with empty vin, and make fundrawtransaction parse the input as old style.

[NicolasDorier]

By quickly checking the code, it seems fundrawtransaction can be used with some inputs, so that the caller can oblige to select some specific coins. (https://github.com/bitcoin/bitcoin/blob/0c95ebce7e67f86f62c099974dde68c8d808240b/src/wallet/wallet.cpp#L1936)

Another reason might be Bob send a partially funded transaction, then give it to Alice to complete the rest.

[sipa]

Nicolas: sure, it can have inputs, but they won't carry signatures.

[NicolasDorier]

I think they might, if it is partially signed by another party for example.

Falling back to classic transaction parsing only seems to be a good solution nonetheless.
With a clear error message in case the caller passed the witness by mistake it should be ok.

[sipa]

@NicolasDorier if fundrawtransaction has any effect at all (add outputs, or change inputs) it will normally invalidate all existing signatures.

[NicolasDorier]

except for AnyOneCanPay signatures. Edge case though.

[sipa]

AnyOneCanPay AND (SigHashNone OR no-change-output added). Agree that we need to support that though, but parsing by default with old-style deserialization, and trying with new-style if it fails will cover everything I expect.

What I wanted to avoid was the need for guessing in general in many RPCs (or worse, P2P code), as there are places where concatenations of transactions can be given, which would lead to an exponential blowup of combimations to try, and hard to define behaviour. As long as it's limited to fundrawtransaction (AFAIK the only place where you can meaningfully pass a transaction without inputs), we're probably fine.

[sipa]

Updated in commit dbe6391. Decoderawtransaction and fundrawtransaction now first try deserializing without witness, and if that fails, or does not consume the entire string that is passed in, try deserializing with the new format. Furthermore, serialization without inputs is reverted to using old serialization as well.

@rustyrussell Does that suffice as a solution for you?

[rustyrussell]

Pieter Wuille [email protected] writes:

Updated in commit dbe6391. Decoderawtransaction and fundrawtransaction now first try deserializing without witness, and if that fails, or does not consume the entire string that is passed in, try deserializing with the new format. Furthermore, serialization without inputs is reverted to using old serialization as well.

@rustyrussell Does that suffice as a solution for you?

Yes. I share your distaste, but fundrawtransaction really is a corner
case.

BTW, I'm surprised tests didn't pick this up...

Ut-Ack: Rusty Russell [email protected]

Thanks!
Rusty.

[sipa]

The tests did pick it. The change was intentional, so I modified them.