protocol/tx: return more specific types from newData and newSpend #475

Merged
merged 1 commit into from Feb 8, 2017

Projects

None yet

6 participants

@bobg
Contributor
bobg commented Feb 4, 2017

Also remove a stale TODO.

@bobg bobg added the PTAL label Feb 4, 2017
@bobg
Contributor
bobg commented Feb 4, 2017

PTAL
I am smol

d := new(data)
d.body = hash
return d
}
func hashData(data []byte) (h bc.Hash) {
- // TODO: Do we want domain separation here? (E.g., a "data:"
- // prefix.) If so, both the code and the spec need updating.
@kr
kr Feb 6, 2017 Member

Did we make a call on this? IMO we should do domain separation as a matter of habit, unless there's a really clear reason not to. cc @tonychain

@tonychain
tonychain Feb 6, 2017 edited Member

I'm definitely a fan of always doing domain separation. The overhead it adds is negligible and it's good security hygiene.

I'd recommend something that looks like URI generic syntax, e.g.:

scheme:data

or barring that, either a fixed-length domain identifier or some sort of delimiter which is not allowed in the domain identifier

@jbowens
jbowens Feb 7, 2017 Member

@kr — I think this was the discussion around it:
https://chainhq.slack.com/archives/txgraph/p1486079399000187

@kr
kr Feb 8, 2017 Member

That the data entry itself serves as a domain separator seems plausible to me. I'll defer to @tonychain and @oleganza.

@oleganza
oleganza Feb 8, 2017 edited Member

Data entry is the domain separator for the raw data. This prevents having the raw hash of the data to be reused in the txgraph structure (meaning, to pose as some valid structure). For the purposes of identifying the raw data using OP_REFDATAHASH we need a raw hash that represents the raw data.

@tonychain
tonychain Feb 8, 2017 Member

To back up a bit, the threat would be an attacker convincing you to sign a message in one context, and then reusing it in another.

So long as that threat is addressed, I'm happy. Sounds like it is?

@oleganza
oleganza Feb 8, 2017 Member

Sounds like it is, yup. Thanks for clarifying the threat model, @tonychain!

@bobg
Contributor
bobg commented Feb 8, 2017

PTAL

@jbowens
jbowens approved these changes Feb 8, 2017 View changes
@jbowens
Member
jbowens commented Feb 8, 2017

lgtm

@bobg @chainbot bobg Remove a stale TODO. Return more specific types from newData and newS…
…pend.
d3744d2
@chainbot chainbot merged commit 17b6cf8 into main Feb 8, 2017

3 checks passed

licence/cla Contributor License Agreement is signed.
Details
wercker/cored Wercker pipeline passed
Details
wercker/java Wercker pipeline passed
Details
@chainbot chainbot deleted the txgraph-tweaks branch Feb 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment