Received: from sog-mx-2.v43.ch3.sourceforge.com ([172.29.43.192] helo=mx.sourceforge.net) by sfs-ml-1.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1UpcU6-0000WH-1t for bitcoin-development@lists.sourceforge.net; Thu, 20 Jun 2013 10:52:58 +0000 Received-SPF: pass (sog-mx-2.v43.ch3.sourceforge.com: domain of gmail.com designates 209.85.223.173 as permitted sender) client-ip=209.85.223.173; envelope-from=pieter.wuille@gmail.com; helo=mail-ie0-f173.google.com; Received: from mail-ie0-f173.google.com ([209.85.223.173]) by sog-mx-2.v43.ch3.sourceforge.com with esmtps (TLSv1:RC4-SHA:128) (Exim 4.76) id 1UpcU4-0002uv-KH for bitcoin-development@lists.sourceforge.net; Thu, 20 Jun 2013 10:52:58 +0000 Received: by mail-ie0-f173.google.com with SMTP id k13so16094116iea.32 for ; Thu, 20 Jun 2013 03:52:51 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.42.47.69 with SMTP id n5mr2975623icf.24.1371725571261; Thu, 20 Jun 2013 03:52:51 -0700 (PDT) Received: by 10.50.149.233 with HTTP; Thu, 20 Jun 2013 03:52:51 -0700 (PDT) Received: by 10.50.149.233 with HTTP; Thu, 20 Jun 2013 03:52:51 -0700 (PDT) In-Reply-To: <1371724625.50978.YahooMailNeo@web162706.mail.bf1.yahoo.com> References: <4DE0E45E-BB48-4DFF-9C86-ACBE312B3049@bitsofproof.com> <20130620090649.GA17765@vps7135.xlshosting.net> <1371724625.50978.YahooMailNeo@web162706.mail.bf1.yahoo.com> Date: Thu, 20 Jun 2013 12:52:51 +0200 Message-ID: From: Pieter Wuille To: Turkey Breast Content-Type: multipart/alternative; boundary=90e6ba6149c667c07804df93bf35 X-Spam-Score: -0.6 (/) X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. -1.5 SPF_CHECK_PASS SPF reports sender host as permitted sender for sender-domain 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (pieter.wuille[at]gmail.com) -0.0 SPF_PASS SPF: sender matches SPF record 1.0 HTML_MESSAGE BODY: HTML included in message -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature X-Headers-End: 1UpcU4-0002uv-KH Cc: Bitcoin Dev Subject: Re: [Bitcoin-development] Missing fRelayTxes in version X-BeenThere: bitcoin-development@lists.sourceforge.net X-Mailman-Version: 2.1.9 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 Jun 2013 10:52:58 -0000 --90e6ba6149c667c07804df93bf35 Content-Type: text/plain; charset=ISO-8859-1 Let's just increase the version number and be done with this discussion. It's a small benefit, but it simplifies things and it's trivial to do. I don't understand how a policy of requiring version increases could limit future extensions: after the version/verack exchange, the protocol version is negotiated between peers, and there is no need for anything optional anymore. Note thay this is just about parsing, not about relaying - you should still relay parts of a message you haven't parsed. But that doesn't apply to the version message anyway, which is the only place where this matters. -- Pieter On 20 Jun 2013 12:38, "Turkey Breast" wrote: > I don't get why this is such a contentious change? > > Before I was able to use asserts to check the expected length of length of > messages per protocol version, I could pass in dumb iterators that just > parse the byte stream and I could serialize and deserialize a message to > check the parser is correct (in debug mode). > > This 'simple' change causes all that behaviour to be lost. You can no > longer just use iterators but must know the remaining length (or if you use > std::distance, you can only use specific std containers - not just anything > with an iterator and an operator++). You cannot check the deserialization > process by serializing the deserialized message and comparing it to the > original data (because the bool is always present in the serializer). > > It's a bit stupid you call it buggy code when this behaviour has never > been present in Bitcoin. The BIP doesn't introduce any unwanted > side-effects and is a trivial reasonable change. > > If you want optional fields then the proper way to do it, is to either set > a flag in the Services field of the "version" message to indicate different > formats for messages (i.e use this template structure for a message, not > that one), introduce a new message (if the changes are big), to > approve/improve Stefan's BIP 32 for custom services or to have a value in > the byte stream indicating which fields are present (maybe a bitfield or > so). > > Using a quirk of an implementation is just bad form and sloppy coding. > Optional fields should have their own mechanism that allows them to remain > as optional fields between protocol version upgrades. > > The bitcoind software can probably be improved too, by checking that the > length of the version message is consistent for the protocol version given > by the connected node. Right now it makes no assumptions based on that > which is a mistake (new clients can broadcast older version messages that > don't have all the fields required). Probably the software should penalise > hosts which do that. > > What's the big deal to update the protocol version number from 70001 to > 70002? It's not like we'll run out of integers. The field has now gone from > optional to required now anyway - that's a behaviour change. It'd be good > to enforce that. I see this as a bug. > > ------------------------------ > *From:* Mike Hearn > *To:* Pieter Wuille > *Cc:* Bitcoin Dev ; Tamas > Blummer > *Sent:* Thursday, June 20, 2013 11:17 AM > *Subject:* Re: [Bitcoin-development] Missing fRelayTxes in version > > There's no problem, but there's no benefit either. It also locks us in to > a potentially problematic guarantee - what if in future we want to have, > say, two optional new pieces of data in two different messages. We don't > want to require that if version > X then you have to implement all features > up to and including that point. > > Essentially the number of fields in a message is like a little version > number, just for that message. It adds flexibility to keep it that way, and > there's no downside, seeing as that bridge was already crossed and people > with parsers that can't handle it need to fix their code anyway. > > So I have a slight preference for keeping things the way they are, it > keeps things flexible for future and costs nothing. > > > > On Thu, Jun 20, 2013 at 11:06 AM, Pieter Wuille wrote: > > On Thu, Jun 20, 2013 at 09:36:40AM +0200, Mike Hearn wrote: > > Sure but why not do that when there's an actual new field to add? Does > > anyone have a proposal for a feature that needs a new version field at > the > > moment? There's no point changing the protocol now unless there's > actually > > a new field to add. > > > > Anyway I still don't see why anyone cares about this issue. The Bitcoin > > protocol does not and never has required that all messages have a fixed > > number of fields per version. Any parser written on the assumption it did > > was just buggy. Look at how tx messages are relayed for the most obvious > > example of that pattern in action - it's actually the raw byte stream > > that's stored and relayed to ensure that fields added in new versions > > aren't dropped during round-tripping. Old versions are supposed to > preserve > > fields from the future. > > Actually, that is not the same issue. What is being argued for here is that > the version in the version message itself should indicate which fields are > present, so a parser doesn't need to look at the length of the message. > That > seems like a minor but very reasonable request to me, and it's trivial to > do. > That doesn't mean that you may receive versions higher than what you know > of, > and thus messages with fields you don't know about. That doesn't matter, > you > can just ignore them. > > I see no problem with raising the protocol version number to indicate > "all fields up to fRelayTxes are required, if the announced nVersion is > above N". > In fact, I believe (though haven't checked) all previous additions to the > version > message were accompanied with a protocol version (then: client version) > increase > as well. > > -- > Pieter > > > > > ------------------------------------------------------------------------------ > This SF.net email is sponsored by Windows: > > Build for Windows Store. > > http://p.sf.net/sfu/windows-dev2dev > _______________________________________________ > Bitcoin-development mailing list > Bitcoin-development@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/bitcoin-development > > > > > ------------------------------------------------------------------------------ > This SF.net email is sponsored by Windows: > > Build for Windows Store. > > http://p.sf.net/sfu/windows-dev2dev > _______________________________________________ > Bitcoin-development mailing list > Bitcoin-development@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/bitcoin-development > > --90e6ba6149c667c07804df93bf35 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

Let's just increase the version number and be done with = this discussion. It's a small benefit, but it simplifies things and it&= #39;s trivial to do.

I don't understand how a policy of requiring version inc= reases could limit future extensions: after the version/verack exchange, th= e protocol version is negotiated between peers, and there is no need for an= ything optional anymore.

Note thay this is just about parsing, not about relaying - y= ou should still relay parts of a message you haven't parsed. But that d= oesn't apply to the version message anyway, which is the only place whe= re this matters.

--
Pieter

On 20 Jun 2013 12:38, "Turkey Breast" = <turkeybreast@yahoo.com>= ; wrote:
I don't get why this is such a contentious change?<= /span>

<= span>Before I was able to use asserts to check the expected length of lengt= h of messages per protocol version, I could pass in dumb iterators that jus= t parse the byte stream and I could serialize and deserialize a message to = check the parser is correct (in debug mode).

<= div style=3D"font-style:normal;font-size:16px;background-color:transparent;= font-family:times new roman,new york,times,serif"> This 'simple' change causes all that behaviour to be lost. Yo= u can no longer just use iterators but must know the remaining length (or i= f you use std::distance, you can only use specific std containers - not jus= t anything with an iterator and an operator++). You cannot check the deseri= alization process by serializing the deserialized message and comparing it = to the original data (because the bool is always present in the serializer)= .

<= div style=3D"font-style:normal;font-size:16px;background-color:transparent;= font-family:times new roman,new york,times,serif"> It's a bit stupid you call it buggy code when this behaviour has = never been present in Bitcoin. The BIP doesn't introduce any unwanted s= ide-effects and is a trivial reasonable change.

<= span>If you want optional fields then the proper way to do it, is to either= set a flag in the Services field of the "version" message to ind= icate different formats for messages (i.e use this template structure for a= message, not that one), introduce a new message (if the changes are big), = to approve/improve Stefan's BIP 32 for custom services or to have a val= ue in the byte stream indicating which fields are present (maybe a bitfield or so).

Using a quirk of an implementation is just bad form and sloppy coding= . Optional fields should have their own mechanism that allows them to remai= n as optional fields between protocol version upgrades.

<= div style=3D"font-style:normal;font-size:16px;background-color:transparent;= font-family:times new roman,new york,times,serif"> The bitcoind software can probably be improved too, by checking that the length of the version message is consistent for = the protocol version given by the connected node. Right now it makes no ass= umptions based on that which is a mistake (new clients can broadcast older = version messages that don't have all the fields required). Probably the= software should penalise hosts which do that.

<= div style=3D"font-style:normal;font-size:16px;background-color:transparent;= font-family:times new roman,new york,times,serif"> What's the big deal to update the protocol version number from 70= 001 to 70002? It's not like we'll run out of integers. The field ha= s now gone from optional to required now anyway - that's a behaviour ch= ange. It'd be good to enforce that. I see this as a bug.


There's no problem, but there's no benefit ei= ther. It also locks us in to a potentially problematic guarantee - what if = in future we want to have, say, two optional new pieces of data in two diff= erent messages. We don't want to require that if version > X then yo= u have to implement all features up to and including that point.

Essentially the number of fields in a message is like a litt= le version number, just for that message. It adds flexibility to keep it th= at way, and there's no downside, seeing as that bridge was already cros= sed and people with parsers that can't handle it need to fix their code= anyway.

So I have a slight preference for keeping things the wa= y they are, it keeps things flexible for future and costs nothing.



On Thu, Jun 20, 2013 at 11:06 AM, Pieter Wuille <pi= eter.wuille@gmail.com> wrote:
On Thu, Jun 20, 2013 at 09:36:40AM +0200, Mike Hearn wrote:
> Sure but why not do that when there's an actual new field to add? = Does
> anyone have a proposal for a feature that needs a new version field at= the
> moment? There's no point changing the protocol now unless there= 9;s actually
> a new field to add.
>
> Anyway I still don't see why anyone cares about this issue. The Bi= tcoin
> protocol does not and never has required that all messages have a fixe= d
> number of fields per version. Any parser written on the assumption it = did
> was just buggy. Look at how tx messages are relayed for the most obvio= us
> example of that pattern in action - it's actually the raw byte str= eam
> that's stored and relayed to ensure that fields added in new versi= ons
> aren't dropped during round-tripping. Old versions are supposed to= preserve
> fields from the future.

Actually, that is not the same issue. What is being argued for here i= s that
the version in the version message itself should indicate which fields are<= br> present, so a parser doesn't need to look at the length of the message.= That
seems like a minor but very reasonable request to me, and it's trivial = to do.
That doesn't mean that you may receive versions higher than what you kn= ow of,
and thus messages with fields you don't know about. That doesn't ma= tter, you
can just ignore them.

I see no problem with raising the protocol version number to indicate
"all fields up to fRelayTxes are required, if the announced nVersion i= s above N".
In fact, I believe (though haven't checked) all previous additions to t= he version
message were accompanied with a protocol version (then: client version) inc= rease
as well.

--
Pieter



-----------------------------------------------------------------= -------------
This SF.net email is sponsored by Windows:

Build fo= r Windows Store.

http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Bitcoin-development mail= ing list
Bitcoin-development@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bitcoin-developmen= t



-----------------------------= -------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.= sf.net/sfu/windows-dev2dev
_________________________________________= ______
Bitcoin-development mailing list
Bitcoin-develo= pment@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bitcoin-de= velopment

--90e6ba6149c667c07804df93bf35--