-
Notifications
You must be signed in to change notification settings - Fork 167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Constant-time scalars in ed25519, and variable-time option flag #173
Conversation
add preliminary support to standard mod.Int and edwards25519
instead of mod.Ints
c11 := (load4(c[28:]) >> 7) | ||
var carry [23]int64 | ||
|
||
s0 := 1916624 - c0 + a0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. I think I understand how you hacked the ScAdd
but this... where does it come from ?
By this I mean this line and most of them below...!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I know this is a bit obscure (like most of this code). :) The constants 1916624, ..., down to s12 := 16, are the representation of the group order (L) times 16, which is the first "representation of 0 mod L" just higher than 2^256. That way, when we subtract c from that, we ensure that the results remains on the positive side overall, which is needed since the modular reduction and carry-handling code below was only set up to handle positive results (but it has no problem handling positive results that are a bit - or a lot - too big).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the second part of your answer but not sure I get the first part. I'll have to dig deeper in how theses computations are done and what are these representations (why s1 -> s23 ) etc. Thanks anyway!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was facing a similar issue a while ago when I was implementing those scalar operations in Rust. To maintain readability and avoid hacky tricks, I decided to write wrappers around the multiply_and_add(a,b,c) { return a*b + c }
method. Basically:
add(a,b) { return multiply_and_add(1,a,b) } // a+b
sub(a,b) { return multiply_and_add(-1,b,a) } // -b + a = a - b
neg(a) { return multiply_and_add(-1,a,0) } // -a
I derived the byte representation of the -1 scalar via the reference implementation (https://ed25519.cr.yp.to/python/ed25519.py) and it should be
0xec, 0xd3, 0xf5, 0x5c, 0x1a, 0x63, 0x12, 0x58,
0xd6, 0x9c, 0xf7, 0xa2, 0xde, 0xf9, 0xde, 0x14,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10,
This needs to be verified again. ;)
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach better: it may cost a few cycles more but at least I understand it, it's way more simpler to grasps ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try benchmarking the two approaches. I suspect that calling multiply_and_add with 1 or -1 will prove a lot slower, because the multiply_and_add will actually do a full multiplication and reduction, whereas in the hacky specialized functions the compiler's constant-propagation will eliminate a ton of that carry-reduction effort in the Add/Sub cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what optimisations golang does if this code would be rewritten in a loop...
Anyway, I'm OK to having magic constants in the code, as long as there is a link to a document that says how they've been chosen (like https://crypto.stackexchange.com/questions/10263/should-we-trust-the-nist-recommended-ecc-parameters)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To double check, I made a quick version of constant time scalar playing with the scMulAdd
parameters to implement Mul
Add
and Sub
. It turns out indeed that @bford 's implementation is two times faster ;)
BenchmarkCTScalarSimpleAdd-4 10000000 196 ns/op
BenchmarkCTScalarAdd-4 20000000 100 ns/op
BenchmarkCTScalarSimpleMul-4 10000000 194 ns/op
BenchmarkCTScalarMul-4 10000000 167 ns/op
BenchmarkCTScalarSimpleSub-4 10000000 193 ns/op
BenchmarkCTScalarSub-4 20000000 98.7 ns/op
PASS
where ...CTScalarSimpleXXX
is my quick implementation. The others are @bford 's implementation.
Nice work. type Scalar interface {
...
// this method can panic if constant time implementation is not available.
// or it can return an error
ConstantTimeScalar() ConstantTimeScalar
}
type ConstantTimeScalar interface {
Scalar
// This should always be possible
VarTimeScalar() Scalar
} Another idea would be to just panic in case the constant time implementation is not available. It may be simpler... |
Thanks Nicolas for the code review. I agree that it's a potential source of confusion that only one of our curve implementations currently supports constant-time operation - but I think for now it's OK to leave it as a well-documented caveat stated loudly and clearly as part of the godoc for the non-constant-time curve implementation modules. In the longer term, it would be great if we can fix that and either make the other implementations constant-time or replace them with better implementations of something else (e.g., Ed448, pairing-based curves, etc...) that do have constant-time implementations. But that can be a project for later. |
I agree that it would nice to have contstant time implementations for other groups as well. In the meantime, I still find it dangerous to not at least panic if the constant time implementation is not implemented yet. Most of the time, we/I/users? don't look at the comments of the concrete implementations so the user can think it's safe while it's not... |
Can you clarify what solution you're proposing? Should the non-constant-time implementations have a 'vartime' flag internally, which defaults to false but just causes panics if it's used that way, so a client who wants to use one of the variable-time implementations must explicitly SetVarTime(true) before using it? That seems potentially reasonable, basically forcing the user to "check the box" that effectively says "I understand and acknowledge that I'm using non-constant-type crypto". :) |
MMhhh right I was not thinking that the default is "constant time" implementations. type Group interface {
Scalar(varTime bool) Scalar
Point(varTime bool) Point
} Both methods can panic at creation time if the variable/constant time implementation is not supported, so there's absolutely no ambiguities from the caller's perspective. However, this approach has the drawback that it changes the API of the Group.. type Group interface {
VarTimeScalar() Scalar
VarTimePoint() Point
Scalar() Scalar
VarTimePoint() Point
} The last two methods ( There's no perfect solutions in any case. I'm more in favor of 1) personnally. |
The idea of defaulting to constant-time was for conservative safety: if you neglect to think about it (or don't understand), you'll get the default constant-time implementation and will at least be "more safe" even if potentially slower than necessary. But I agree that defaulting to constant-time in implementations that don't have constant-time at all is a bit problematic. :) I'm reluctant to add complexity to the Scalar/Point constructors in the Group interface, because the "ideal situation" I'd really like to get to is one where usually you never worry about or deal with the varTime flag at all, and instead you just almost always use the default constant-time implementations of everything (once everything has constant-time implementations :) ) - except in the rare performance-critical cases where you're performance-optimizing a particular algorithm based on our crypto library (such as signature verification or ZKP verification) and realize that no secrets are actually involved in that calculation, so you add the one or two SetVarTime(true) calls as appropriate to speed up the most performance-critical parts of those calculations. I'd rather not have everybody have to think about constant- versus vartime all the time and pass the right parameters or call the right flavor of Point/Scalar constructor. Better just to default to something safe and provide an uncommon-case override for when the client is really sure it's OK and performance-beneficial to use vartime operations. |
group/edwards25519/point.go
Outdated
//var a [32]byte | ||
//for i := range sb { | ||
// a[shi-i] = sb[i] | ||
//} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is not used anymore, please delete it.
group/mod/int.go
Outdated
@@ -108,7 +108,8 @@ func (i *Int) InitString(n, d string, base int, m *big.Int) *Int { | |||
|
|||
// Return the Int's integer value in decimal string representation. | |||
func (i *Int) String() string { | |||
return hex.EncodeToString(i.V.Bytes()) | |||
return i.V.String() | |||
//return hex.EncodeToString(i.V.Bytes()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove line.
group/mod/int.go
Outdated
@@ -473,3 +474,11 @@ func (i *Int) HideDecode(buf []byte) { | |||
i.V.SetBytes(buf) | |||
i.V.Mod(&i.V, i.M) | |||
} | |||
|
|||
// Allow or disallow variable-time implementation. | |||
// This implementation unfortunately provices only variable-time operations, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/provices/provides/
Chiming in late to the discussion. If I understand correctly, we want to avoid that people use our library and think it is 'safe' but then leak information about their private keys.
I can think of two more:
|
@ineiti Good ideas. I like 3. since it's clear and easy to grasp. We can make the default For 4., it's also a good idea but I have trouble to see where this global variable would be defined. Is it in the code ? or during the building phase (like C/C++ projects compilation option -DMY_VARIABLE ?
|
I don't like #3 so much because it effectively separates constant and variable-time implementations into completely different "cipher suites", which is semantically inappropriate and in its logical conclusion may make them extremely cumbersome to use. For example, if you have to instantiate different cipher-suites for constant-time or variable-time implementations of a curve, can you or can't you expect Scalars and Points created from one implementation to be accepted as inputs to operations on the other? If they're different cipher suites, it would seem most natural to assume/require clients to expect they might be incompatible. Thus, to sign and verify signatures, the signing code will need to instantiate a constant-time ciphersuite and load the public key into a Point instance of that, whereas the verifying code will have to instantiate the other variable-time ciphersuite and load the (same!) public key into a (different) Point instance from that, just so that verification (a non-sensitive public process) can take advantage of the variable-time operations while keeping the signing process secure. |
And you KNOW how I feel about global variables!!! |
We could do something like this: https://husobee.github.io/golang/compile/time/variables/2015/12/03/compile-time-const.html But it'll still be a variable somewhere in
But that might be another case of cuddling the users. Even though I'm all for cuddling ;) As I see now the problem with my proposition for 3. (even though perhaps some nice use of |
Actually, I think the Go standard approach to this would be just to make all the non-constant-time curve implementations (i.e., currently all but ed25519) conditionally compiled on a predicate such as '!vartime-curve-implementations'. I think that would probably be a reasonable solution, and would effectively slim down the default compilation of the Kyber liberary even more. I don't think we're currently using anything but ed25519 in "production" for onet etc anyway, are we? (If we are, we probably shouldn't be!) |
OK, missed those flags - I never used them. But yes, this also sounds very reasonable to me. And yes, for the moment we only use ed25519 in onet/cothority. That's the advantage of having a global variable (just kidding, it'll go away in v2)! |
I've added constant time implementation for the inversion (so Div is now also in constant time). |
group/curve25519/ext.go
Outdated
@@ -267,6 +269,11 @@ func (P *extPoint) Mul(s kyber.Scalar, G kyber.Point) kyber.Point { | |||
return P | |||
} | |||
|
|||
// This implementation only supports variable-time operations | |||
func (P *extPoint) SetVarTime(varTime bool) bool { | |||
return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably also fatal
if varTime
is false
.
group/edwards25519/curve.go
Outdated
|
||
// Set to true to use the full group of order 8Q, | ||
// or false to use the prime-order subgroup of order Q. | ||
// FullGroup bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still available and should be re-enabled?
group/edwards25519/curve.go
Outdated
|
||
// Create a new Scalar for the prime-order subgroup of the Ed25519 curve. | ||
func (c *Curve) Scalar() kyber.Scalar { | ||
//i := mod.NewInt64(0, primeOrder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused code - remove or re-enable?
group/edwards25519/curve.go
Outdated
// Create a new Point on the Ed25519 curve. | ||
func (c *Curve) Point() kyber.Point { | ||
P := new(point) | ||
//P.c = c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused code
group/edwards25519/scalar.go
Outdated
"gopkg.in/dedis/kyber.v1/util/random" | ||
"gopkg.in/dedis/kyber.v1/util/subtle" | ||
) | ||
|
||
// This code is a port of the public domain, "ref10" implementation of ed25519 | ||
// from SUPERCOP. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add reference to supercop:
https://bench.cr.yp.to/supercop.html
and perhaps:
https://github.com/anonimal/supercop-ed25519-ref10
group/edwards25519/scalar.go
Outdated
return s | ||
} | ||
|
||
func (s *scalar) Clone() kyber.Scalar { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint and gofmt do not complain. it implements the kyber.Scalar interface so it should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it's a private structure...
group/edwards25519/scalar.go
Outdated
} | ||
|
||
// Set to a small integer value | ||
func (s *scalar) SetInt64(v int64) kyber.Scalar { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
golint error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even if golint doesn't complain - can we still follow go's convention for commenting methods? Please? Pretty Please?
} | ||
|
||
func (s *scalar) SetBytes(b []byte) kyber.Scalar { | ||
// XXX handle simple and scReduce cases appropriately |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a todo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scReduce
takes only 64 bytes as input, so I'm not sure we should use it when given more than 64 bytes in SetBytes
. Otherwise, we need to cut down the given slice of bytes and that may yield wrong results ?. i would keep it the way it is now. What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even 64 bytes might be too big for ed25519-scalars. I did not understand the difference between simple
and scReduce
cases.
group/edwards25519/scalar.go
Outdated
} | ||
|
||
func (s *scalar) SetVarTime(varTime bool) bool { | ||
return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is only a constant-time implementation? Should it also fatal
when called with true
? Or is it OK to accept true
because that's not a security-problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should just always silently return false, as per the current code. This way:
-
a client who wants to test whether a given Point or Scalar implementation supports constant time at all can see if s.SetVarTime(false).SetVarTime(false) returns false (it's constant-time) or true (it only has a variable-time implementation).
-
a client who wants to test whether a constant-time Point or Scalar implementation supports any variable-time optimizations can test the result of s.SetVarTime(true).SetVarTime(true): if false, it's always constant-time; if true, it is either always or configurably variable-time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds method-overloading to me. I'd prefer to have a GetVarTime
method for that.
Another proposition would be to have a (bool, error)
-return, so the user is correctly informed if his desired action has been performed or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer avoiding adding again a new method to Scalar
if possible. The interface starts to get really long.
Why again do we need to return the old "varTime" value ? Is it needed in practice ? I would have just imagine some part of the code that needs vartime to just call scalar.SetVarTime(true)
, why should they care about the old value ?
If we make that method return an error as @ineiti suggested, it becomes if err := scalar.SetVarTime(true); err != nil { ... }
. What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds nice to me - what does @bford think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting approval by @bford before implementing or not ? (I already reverted many times some changes so I'd prefer to wait until then :D )
group/edwards25519/scalar.go
Outdated
|
||
func (s *scalar) MarshalBinary() ([]byte, error) { | ||
return s.toInt().MarshalBinary() | ||
//buf := s.v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused code
group/mod/int.go
Outdated
@@ -108,6 +108,7 @@ func (i *Int) InitString(n, d string, base int, m *big.Int) *Int { | |||
|
|||
// Return the Int's integer value in decimal string representation. | |||
func (i *Int) String() string { | |||
//return i.V.String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused code
Things to do:
|
s21 := int64(0) | ||
s22 := int64(0) | ||
s23 := int64(0) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid code duplication, should we modularize the code below into "carry-center/uncenter" and "reduce" functions similarly to https://github.com/isislovecruft/curve25519-dalek/blob/master/src/scalar.rs#L607 ff and then use them in scAdd()
, scSub()
, etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could try doing that, but be sure to benchmark before/after, as I expect the obvious/easy ways of doing that will disable or make it more difficult for the compiler to optimize it down using constant propagation etc. That's the main reason I didn't try to reduce code duplication in that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for this idea. I like having the more public methods on top of the file (scAdd
, scSub
) and the helpers (center/uncenter
, reduce
) at the bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bford Indeed you were completely right !
BenchmarkCTScalarAdd-4 20000000 104 ns/op
BenchmarkCTScalarSimpleAdd-4 10000000 196 ns/op
BenchmarkCTScalarFactoredAdd-4 2000000 592 ns/op
BenchmarkCTScalarMul-4 10000000 166 ns/op
BenchmarkCTScalarSimpleMul-4 10000000 200 ns/op
BenchmarkCTScalarFactoredMul-4 2000000 722 ns/op
BenchmarkCTScalarSub-4 20000000 98.5 ns/op
BenchmarkCTScalarSimpleSub-4 10000000 192 ns/op
BenchmarkCTScalarFactoredSub-4 2000000 610 ns/op
where ScalarFactoredXXX
is is using a factored out version of scMul
(etc), and ScalarSImpleXXX
is using the trick with ScMulAdd
by passing -1
or 0
to implement the operations.
Crazy thought the big difference in time. I'd love to see this kind of benchmark on Rust as well... And also to take some compiler courses to really grasp the thing > < :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, taking a compiler course would be worthwhile - you'll immediately understand the reasons behind performance effects like this. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No simplification then...
cipher/stream.go
Outdated
@@ -80,10 +80,11 @@ func (sc *streamCipher) Partial(dst, src, key []byte) { | |||
// absorb cryptographic input (which may overlap with dst) | |||
if key != nil { | |||
nkey := ints.Min(n, len(key)) // # key bytes available | |||
sc.h.Write(key[:nkey]) | |||
// XXX check return error ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in fact an error-check here would be very nice. But as the main method does not return any error, I'm not sure what is best to do here.
So either remove the _, _ =
or do some error-checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the XXX
can go away.
@@ -54,7 +54,7 @@ func SchnorrSign(suite Suite, random cipher.Stream, message []byte, | |||
// And check that hashElgamal for T and the message == c | |||
buf := bytes.Buffer{} | |||
sig := basicSig{c, r} | |||
suite.Write(&buf, &sig) | |||
_ = suite.Write(&buf, &sig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again error-checking: either remove _ =
or do some error-checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint.
@@ -87,9 +87,9 @@ func SchnorrVerify(suite Suite, message []byte, publicKey kyber.Point, | |||
} | |||
|
|||
// Example of using Schnorr | |||
func ExampleSchnorr() { | |||
func Example_schnorr() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow https://golang.org/pkg/testing/#pkg-examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golint do not pass with that so I choose golint.
Caught Bryan and asked him for |
Piouf ! That might be the end of that PR ? :D |
This PR implements fixes several issues that I think have been needing to get taken care of for a while:
The biggest change, which motivates and drives the others, is in group/edwards25519, where I added real, constant-time scalar arithmetic for this curve, based on djb's code but adapted to support the generic Add/Sub/Mul operations we need rather than just the MulAdd and Reduce operations that come in the ed25519 signature scheme impl. The Inv and Div operations are still non-constant time, unfortunately; that probably can and should be fixed too but will have to wait.
Since constant-time operation often comes at a performance cost, and for example the Ed25519 point arithmetic code already includes both constant-time and non-constant-time versions of the main scalar multiply operation, I made those accessible by adding a SetVarTime() method to both the Point and Scalar interfaces. The idea is that any implementation should default to constant-time implementations of all operations if constant-time operations are available; however, the caller can invoke SetVarTime(true) on particular Points and/or Scalars to indicate that operations on those particular Point/Scalar objects will never utilize secret information and hence should be safe to run in non-constant-time if that's faster.
Added an endianness parameter to mod.Int.InitBytes(), because without it there's no way to specify the endianness the byte slice the caller is passing to this function. (Since the stateful endianness of the object itself hasn't been initialized yet.) But also see this more general related issue: Bytes/SetBytes endianness control #172
The Clone() operations in curv25519/proj.go and curve25519/ext.go were buggy, causing incorrect state-sharing in ways that apparently snuck past the test suites before (but for some reason got revealed while I was working on a completely different package)... Fixed.
The Add and Sub methods for projPoint in curve25519 similarly had state-overwrite bugs causing corruption when the output overlapped with one or both input objects; again this apparently slipped past the test suites before.
This code is NOT yet ready to commit; while the normal arithmetic tests pass, the EdDSA signing test using it doesn't yet succeed, so there's something still not quite right with it. But I wanted to get review/discussion started on the API changes at least. (This is for v1 of course.)