Date: Tue, 26 Jun 2018 17:56:26 -0400
To: matejcik <jan.matejek@satoshilabs.com>,
	Bitcoin Protocol Discussion <bitcoin-dev@lists.linuxfoundation.org>
From: Achow101 <achow101-lists@achow101.com>
Reply-To: Achow101 <achow101-lists@achow101.com>
Message-ID: <J0KV-aP96fSVHPkPw85N2qdKV_F5vqXt5fIFwKDp9wBjRKJ6bZpUEtzbgHyxlWW9PCXMOEVZnyUnJ-kW281ZbblbCp2sbZI_UyTP46q-PiY=@achow101.com>
In-Reply-To: <c32dc90d-9919-354b-932c-f93fe329760b@satoshilabs.com>
References: <CAPg+sBhGMxXatsyCAqeboQKH8ASSFAfiXzxyXR9UrNFnah5PPw@mail.gmail.com>
	<CHCiA27GTRiVfkF1DoHdroJL1rQS77ocB42nWxIIhqi_fY3VbB3jsMQveRJOtsJiA4RaCAVe3VZmLZsXVYS3A5wVLNP2OgKQiHE0T27P2qc=@achow101.com>
	<21a616f5-7a17-35b9-85ea-f779f20a6a2d@satoshilabs.com>
	<20180621195654.GC99379@coinkite.com>
	<CAPg+sBgdQqZ8sRSn=dd9EkavYJA6GBiCu6-v5k9ca-9WLPp72Q@mail.gmail.com>
	<ljk5Z_a3KK6DHfmPJxI8o9W2CkwszkUG34h0i1MTGU4ss8r3BTQ3GnTtDTfWF6J7ZqcSAmejzrr11muWqYN-_wnWw_0NFn5_lggNnjI0_Rc=@achow101.com>
	<f8f5b1e3-692a-fc1e-2ad3-c4ad4464957f@satoshilabs.com>
	<TGyS7Azu3inMQFv9QFn8USr9v2m5QbhDRmiOI-4FWwscUeuIB9rA7mCmZA4-kwCJOMAx92fO7XICHtE7ES_QmIYLDy6RHof1WLALskGUYAc=@achow101.com>
	<c32dc90d-9919-354b-932c-f93fe329760b@satoshilabs.com>
Feedback-ID: VjS95yl5HLFwBfNLRqi61OdL1ERZPmvMbZRH2ZcBR7SKVUVYPgv7VJsV9uoyC4vIfjYnW8hPXGuLTycZbh49Zw==:Ext:ProtonMail
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [bitcoin-dev] BIP 174 thoughts
Precedence: list

Hi,

On June 26, 2018 8:33 AM, matejcik via bitcoin-dev <bitcoin-dev@lists.linux=
foundation.org> wrote:

> =E2=80=8B=E2=80=8B
>=20
> hello,
>=20
> in general, I agree with my colleague Tomas, the proposed changes are
>=20
> good and achieve the most important things that we wanted. We'll review
>=20
> the proposal in more detail later.
>=20
> For now a couple minor things I have run into:
>=20
> -   valid test vector 2 ("one P2PKH input and one P2SH-P2WPKH input")
>    =20
>     seems broken, at least its hex version; a delimiter seems to be missi=
ng,
>    =20
>     misplaced or corrupted

Fixed

>    =20
> -   at least the first signing vector is not updated, but you probably
>    =20
>     know that

I updated all of the tests yesterday so they should be correct now. I will =
be adding more tests
this week.

>    =20
> -   BIP32 derivation fields don't specify size of the "master public key"=
,
>    =20
>     which would make it un-parsable :)

Oops, that's actually supposed to be master key fingerprint, not master pub=
lic key. I have updated
the BIP to reflect this.

>    =20
> -   "Transaction format is specified as follows" and its table need to be
>    =20
>     updated.

Done.

>    =20
>     I'm still going to argue against the key-value model though.
>    =20
>     It's true that this is not significant in terms of space. But I'm mor=
e
>    =20
>     concerned about human readability, i.e., confusing future implementer=
s.
>    =20
>     At this point, the key-value model is there "for historical reasons",
>    =20
>     except these aren't valid even before finalizing the format. The
>    =20
>     original rationale for using key-values seems to be gone (no key-base=
d
>    =20
>     lookups are necessary). As for combining and deduplication, whether k=
ey
>    =20
>     data is present or not is now purely a stand-in for a "repeatable" fl=
ag.
>    =20
>     We could just as easily say, e.g., that the high bit of "type" specif=
ies
>    =20
>     whether this record can be repeated.
>    =20
>     (Moreover, as I wrote previously, the Combiner seems like a weirdly
>    =20
>     placed role. I still don't see its significance and why is it importa=
nt
>    =20
>     to correctly combine PSBTs by agents that don't understand them. If y=
ou
>    =20
>     have a usecase in mind, please explain.

There is a diagram in the BIP that explains this. The combiner's job is to =
combine two PSBTs that
are for the same transaction but contain different data such as signatures.=
 It is easier to implement
a combiner that does not need to understand the types at all, and such comb=
iners are forwards compatible,
so new types do not require new combiner implementations.

>    =20
>     ISTM a Combiner could just as well combine based on whole-record
>    =20
>     uniqueness, and leave the duplicate detection to the Finalizer. In ca=
se
>    =20
>     the incoming PSBTs have incompatible unique fields, the Combiner woul=
d
>    =20
>     have to fail anyway, so the Finalizer might as well do it. Perhaps it
>    =20
>     would be good to leave out the Combiner role entirely?)

A transaction that contains duplicate keys would be completely invalid. Fur=
thermore, in the set of typed
records model, having more than one redeemScript and witnessScript should b=
e invalid, so a combiner
would still need to understand what types are in order to avoid this situat=
ion. Otherwise it would produce
an invalid PSBT.

I also dislike the idea of having type specific things like "only one redee=
mScript" where a more generic
thing would work.

>    =20
>     There's two remaining types where key data is used: BIP32 derivations
>    =20
>     and partial signatures. In case of BIP32 derivation, the key data is
>    =20
>     redundant ( pubkey =3D derive(value) ), so I'd argue we should leave =
that
>    =20
>     out and save space.=20

I think it is still necessary to include the pubkey as not all signers who =
can sign for a given pubkey may
know the derivation path. For example, if a privkey is imported into a wall=
et, that wallet does not necessarily
know the master key fingerprint for the privkey nor would it necessarily ha=
ve the master key itself in
order to derive the privkey. But it still has the privkey and can still sig=
n for it.

>    =20
>     Re breaking change, we are proposing breaking changes anyway, existin=
g
>    =20
>     code will need to be touched, and given that this is a hand-parsed
>    =20
>     format, changing `parse_keyvalue` to `parse_record` seems like a smal=
l
>    =20
>     matter?

The point is to not make it difficult for existing implementations to chang=
e. Mostly what has been done now is just
moving things around, not an entire format change itself. Changing to a set=
 of typed records model require more
changes and is a complete restructuring of the format, not just moving thin=
gs around.

Additionally, I think that the current model is fairly easy to hand parse. =
I don't think a record set model would make
it easier to follow. Furthermore, moving to Protobuf would make it harder t=
o hand parse as varints are slightly more
confusing in protobuf than with Compact size uints. And with the key-value =
model, you don't need to know the types
to know whether something is valid. You don't need to interpret any data.


Andrew