* [bitcoin-dev] SegWit GBT updates
@ 2016-01-30 18:50 Luke Dashjr
2016-02-01 18:41 ` Cory Fields
0 siblings, 1 reply; 7+ messages in thread
From: Luke Dashjr @ 2016-01-30 18:50 UTC (permalink / raw)
To: Bitcoin Dev
I've completed an initial draft of a BIP for updating getblocktemplate for
segregated witness here:
https://github.com/luke-jr/bips/blob/segwit_gbt/bip-segwit-gbt.mediawiki
Please review and comment (especially with regard to the changes in the
sigoplimits handling).
(Note: libblkmaker's reference implementation is at this time incompatible
with the "last output" rule in this BIP.)
Thanks,
Luke
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bitcoin-dev] SegWit GBT updates 2016-01-30 18:50 [bitcoin-dev] SegWit GBT updates Luke Dashjr @ 2016-02-01 18:41 ` Cory Fields 2016-02-01 19:46 ` Luke Dashjr 0 siblings, 1 reply; 7+ messages in thread From: Cory Fields @ 2016-02-01 18:41 UTC (permalink / raw) To: Luke Dashjr; +Cc: Bitcoin Dev Thanks for getting this started, Luke. Noticeably absent here is the "default_witness_commitment" key, as added by the current reference implementation[0]. I assume (please correct me if I'm wrong) that this has been omitted for the sake of having clients create the commitment themselves as opposed to having it provided to them. I don't think that the two approaches (providing the default commitment for the complete tx set as well as the ability to create it from chosen transactions) are at odds with each-other, rather it merely allows for a simpler approach for those who are taking tx's as-is from bitcoind. It's obviously important for the clients to be able to chose tx's and create commitments as they desire, but it's equally important to allow for simpler use-cases. The issue in particular here is that a non-trivial burden is thrust upon mining software, increasing the odds of bugs in the process. I'd like to point out that this is not a theoretical argument. I've already fixed a handful of bugs relating to serialization or commitment creation in the mining/pool software that I've worked on for segwit [1][2][3][4]. Asking them to handle more serialization and calculation of complex structures needlessly increases the complexity for zero benefit in the case where the tx's are to be used as-is. I'll PR this change to the BIP, as I can't really come up with an argument against. At worst, it can simply be ignored. [0]: https://github.com/sipa/bitcoin/blob/segwit/src/rpcmining.cpp#L590 [1]: https://github.com/bitcoin/libblkmaker/commit/22f6e42844aa14ed0037ebf12a734f07e63533d7 [2]: https://github.com/bitcoin/libblkmaker/commit/15e2c35bf69c997488e37147cf062dfa925b4912 [3]: https://github.com/bitcoin/libblkmaker/commit/9a5799891e0f3590779b8e5a993a7b306088e2fa [4]: https://github.com/theuni/ckpool/commit/7d84b1d76b39591cc1c1ef495ebec513cb19a08e Regards, Cory On Sat, Jan 30, 2016 at 1:50 PM, Luke Dashjr via bitcoin-dev <bitcoin-dev@lists•linuxfoundation.org> wrote: > I've completed an initial draft of a BIP for updating getblocktemplate for > segregated witness here: > https://github.com/luke-jr/bips/blob/segwit_gbt/bip-segwit-gbt.mediawiki > > Please review and comment (especially with regard to the changes in the > sigoplimits handling). > > (Note: libblkmaker's reference implementation is at this time incompatible > with the "last output" rule in this BIP.) > > Thanks, > > Luke > _______________________________________________ > bitcoin-dev mailing list > bitcoin-dev@lists•linuxfoundation.org > https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bitcoin-dev] SegWit GBT updates 2016-02-01 18:41 ` Cory Fields @ 2016-02-01 19:46 ` Luke Dashjr 2016-02-01 21:43 ` Cory Fields 0 siblings, 1 reply; 7+ messages in thread From: Luke Dashjr @ 2016-02-01 19:46 UTC (permalink / raw) To: Cory Fields; +Cc: Bitcoin Dev On Monday, February 01, 2016 6:41:06 PM Cory Fields wrote: > Noticeably absent here is the "default_witness_commitment" key, as > added by the current reference implementation[0]. > > I assume (please correct me if I'm wrong) that this has been omitted > for the sake of having clients create the commitment themselves as > opposed to having it provided to them. > > I don't think that the two approaches (providing the default > commitment for the complete tx set as well as the ability to create it > from chosen transactions) are at odds with each-other, rather it > merely allows for a simpler approach for those who are taking tx's > as-is from bitcoind. It's obviously important for the clients to be > able to chose tx's and create commitments as they desire, but it's > equally important to allow for simpler use-cases. Allowing for simpler cases both encourages the lazy case, and enables pools to require miners use it. It also complicates the server-side implementation somewhat, and could in some cases make it more vulnerable to DoS attacks. Keep in mind that GBT is not merely a bitcoind protocol, but is used between pool<->miner as well... For now, it makes sense to leave "default_witness_commitment" as a bitcoind-specific extension to encourage adoption, but it seems better to leave it out of the standard protocol. Let me know if this makes sense or if I'm overlooking something. > The issue in particular here is that a non-trivial burden is thrust > upon mining software, increasing the odds of bugs in the process. It can always use libblkmaker to handle the "heavy lifting"... In any case, the calculation for the commitment isn't significantly more than what it must already do for the stripped merkle tree. > I'd like to point out that this is not a theoretical argument. I've > already fixed a handful of bugs relating to serialization or > commitment creation in the mining/pool software that I've worked on > for segwit [1][2][3][4]. That's not really fair IMO. I wrote the libblkmaker branch prior to even reading the SegWit BIPs or code, and without a way to test it. It's only to be expected there are bugs that get fixed in first-try testing. > [4]: > https://github.com/theuni/ckpool/commit/7d84b1d76b39591cc1c1ef495ebec513cb > 19a08e I'm pretty sure this commit is actually /introducing/ a bug in working (albeit ugly) code. The height, while always positive, is serialised as a signed number, so 0x80 needs to be two bytes: 80 00. Luke ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bitcoin-dev] SegWit GBT updates 2016-02-01 19:46 ` Luke Dashjr @ 2016-02-01 21:43 ` Cory Fields 2016-02-01 23:08 ` Luke Dashjr 0 siblings, 1 reply; 7+ messages in thread From: Cory Fields @ 2016-02-01 21:43 UTC (permalink / raw) To: Luke Dashjr; +Cc: Bitcoin Dev On Mon, Feb 1, 2016 at 2:46 PM, Luke Dashjr <luke@dashjr•org> wrote: > On Monday, February 01, 2016 6:41:06 PM Cory Fields wrote: >> Noticeably absent here is the "default_witness_commitment" key, as >> added by the current reference implementation[0]. >> >> I assume (please correct me if I'm wrong) that this has been omitted >> for the sake of having clients create the commitment themselves as >> opposed to having it provided to them. >> >> I don't think that the two approaches (providing the default >> commitment for the complete tx set as well as the ability to create it >> from chosen transactions) are at odds with each-other, rather it >> merely allows for a simpler approach for those who are taking tx's >> as-is from bitcoind. It's obviously important for the clients to be >> able to chose tx's and create commitments as they desire, but it's >> equally important to allow for simpler use-cases. > > Allowing for simpler cases both encourages the lazy case, and enables pools to > require miners use it. It also complicates the server-side implementation > somewhat, and could in some cases make it more vulnerable to DoS attacks. Keep > in mind that GBT is not merely a bitcoind protocol, but is used between > pool<->miner as well... For now, it makes sense to leave > "default_witness_commitment" as a bitcoind-specific extension to encourage > adoption, but it seems better to leave it out of the standard protocol. Let me > know if this makes sense or if I'm overlooking something. > I think that's a bit of a loaded answer. What's to keep a pool from building its own commitment and requiring miners to use that? I don't see how providing the known-working commitment for the passed-in-hashes allows the pool/miner to do anything they couldn't already, with the exception of skipping some complexity. Please don't confuse encouraging with enabling. What's the DoS vector here? >> The issue in particular here is that a non-trivial burden is thrust >> upon mining software, increasing the odds of bugs in the process. > > It can always use libblkmaker to handle the "heavy lifting"... In any case, > the calculation for the commitment isn't significantly more than what it must > already do for the stripped merkle tree. Agreed. However for the sake of initial adoption, it's much easier to have a known-correct value to use. Even if it's just for the sake of checking against. > >> I'd like to point out that this is not a theoretical argument. I've >> already fixed a handful of bugs relating to serialization or >> commitment creation in the mining/pool software that I've worked on >> for segwit [1][2][3][4]. > > That's not really fair IMO. I wrote the libblkmaker branch prior to even > reading the SegWit BIPs or code, and without a way to test it. It's only to be > expected there are bugs that get fixed in first-try testing. I didn't mean this as an insult/attack, quite the opposite actually. Thanks for doing the integration :) I was merely pointing out how easy it is to introduce subtle bugs here. > >> [4]: >> https://github.com/theuni/ckpool/commit/7d84b1d76b39591cc1c1ef495ebec513cb >> 19a08e > > I'm pretty sure this commit is actually /introducing/ a bug in working (albeit > ugly) code. The height, while always positive, is serialised as a signed > number, so 0x80 needs to be two bytes: 80 00. You're right, thanks. The current code breaks on heights of (for ex) 16513. I'll fix up my changes to take the sign bit into account. Heh, that only reinforces my point above about introducing bugs :p > > Luke ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bitcoin-dev] SegWit GBT updates 2016-02-01 21:43 ` Cory Fields @ 2016-02-01 23:08 ` Luke Dashjr 2016-02-02 1:40 ` Cory Fields 0 siblings, 1 reply; 7+ messages in thread From: Luke Dashjr @ 2016-02-01 23:08 UTC (permalink / raw) To: Cory Fields; +Cc: Bitcoin Dev On Monday, February 01, 2016 9:43:33 PM Cory Fields wrote: > On Mon, Feb 1, 2016 at 2:46 PM, Luke Dashjr <luke@dashjr•org> wrote: > > Allowing for simpler cases both encourages the lazy case, and enables > > pools to require miners use it. It also complicates the server-side > > implementation somewhat, and could in some cases make it more vulnerable > > to DoS attacks. Keep in mind that GBT is not merely a bitcoind protocol, > > but is used between pool<->miner as well... For now, it makes sense to > > leave > > "default_witness_commitment" as a bitcoind-specific extension to > > encourage adoption, but it seems better to leave it out of the standard > > protocol. Let me know if this makes sense or if I'm overlooking > > something. > > I think that's a bit of a loaded answer. What's to keep a pool from > building its own commitment and requiring miners to use that? I don't > see how providing the known-working commitment for the > passed-in-hashes allows the pool/miner to do anything they couldn't > already, with the exception of skipping some complexity. Please don't > confuse encouraging with enabling. Making it simpler to do a centralised implementation than a decentralised one, is both enabling and encouraging. GBT has always been designed to make it difficult to do in a centralised manner. > What's the DoS vector here? It's more work for the pool to provide it, similar to the "midstate" field was with getwork. Someone performing a DoS needs to do less work to force the pool to do complex calculations (unless the same transaction tree / commitment is used for all miners, which would be an unfortunate limitation). > >> The issue in particular here is that a non-trivial burden is thrust > >> upon mining software, increasing the odds of bugs in the process. > > > > It can always use libblkmaker to handle the "heavy lifting"... In any > > case, the calculation for the commitment isn't significantly more than > > what it must already do for the stripped merkle tree. > > Agreed. However for the sake of initial adoption, it's much easier to > have a known-correct value to use. Even if it's just for the sake of > checking against. Sure, I'm not suggesting we remove this from bitcoind (probably the only place that makes initial adoption easier). > >> [4]: > >> https://github.com/theuni/ckpool/commit/7d84b1d76b39591cc1c1ef495ebec513 > >> cb 19a08e > > > > I'm pretty sure this commit is actually /introducing/ a bug in working > > (albeit ugly) code. The height, while always positive, is serialised as > > a signed number, so 0x80 needs to be two bytes: 80 00. > > You're right, thanks. The current code breaks on heights of (for ex) > 16513. I'll fix up my changes to take the sign bit into account. I'm curious what bug it was fixing? Was it overwriting data beyond the number? Luke ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bitcoin-dev] SegWit GBT updates 2016-02-01 23:08 ` Luke Dashjr @ 2016-02-02 1:40 ` Cory Fields 2016-02-02 2:30 ` Luke Dashjr 0 siblings, 1 reply; 7+ messages in thread From: Cory Fields @ 2016-02-02 1:40 UTC (permalink / raw) To: Luke Dashjr; +Cc: Bitcoin Dev On Mon, Feb 1, 2016 at 6:08 PM, Luke Dashjr <luke@dashjr•org> wrote: > On Monday, February 01, 2016 9:43:33 PM Cory Fields wrote: >> On Mon, Feb 1, 2016 at 2:46 PM, Luke Dashjr <luke@dashjr•org> wrote: >> > Allowing for simpler cases both encourages the lazy case, and enables >> > pools to require miners use it. It also complicates the server-side >> > implementation somewhat, and could in some cases make it more vulnerable >> > to DoS attacks. Keep in mind that GBT is not merely a bitcoind protocol, >> > but is used between pool<->miner as well... For now, it makes sense to >> > leave >> > "default_witness_commitment" as a bitcoind-specific extension to >> > encourage adoption, but it seems better to leave it out of the standard >> > protocol. Let me know if this makes sense or if I'm overlooking >> > something. >> >> I think that's a bit of a loaded answer. What's to keep a pool from >> building its own commitment and requiring miners to use that? I don't >> see how providing the known-working commitment for the >> passed-in-hashes allows the pool/miner to do anything they couldn't >> already, with the exception of skipping some complexity. Please don't >> confuse encouraging with enabling. > > Making it simpler to do a centralised implementation than a decentralised one, > is both enabling and encouraging. GBT has always been designed to make it > difficult to do in a centralised manner. > But your suggestion is "use libblkmaker" which will build the trees for me. By that logic, isn't libblkmaker making a centralized implementation easier? Shouldn't that usage be discouraged as well? And along those lines, shouldn't the fact that it's used as a pool <-> miner protocol be discouraged rather than touted as a feature? I don't wish to sound hostile, I'm just trying follow the logic. I can't rationalize why GBT shouldn't expose the commitment that it knows to be correct (when paired with the transactions it provides), purely to make things difficult. >> What's the DoS vector here? > > It's more work for the pool to provide it, similar to the "midstate" field was > with getwork. Someone performing a DoS needs to do less work to force the pool > to do complex calculations (unless the same transaction tree / commitment is > used for all miners, which would be an unfortunate limitation). It's being provided to them. And if they're using a modified set of tx's, they'll need to re-calculate it in order to verify the result anyway. I suspect I'm not understanding this argument. > >> >> The issue in particular here is that a non-trivial burden is thrust >> >> upon mining software, increasing the odds of bugs in the process. >> > >> > It can always use libblkmaker to handle the "heavy lifting"... In any >> > case, the calculation for the commitment isn't significantly more than >> > what it must already do for the stripped merkle tree. >> >> Agreed. However for the sake of initial adoption, it's much easier to >> have a known-correct value to use. Even if it's just for the sake of >> checking against. > > Sure, I'm not suggesting we remove this from bitcoind (probably the only place > that makes initial adoption easier). > How about exposing it as a feature/capability, then? That way pools can expect it from bitcoind, but won't be required to expose it downstream. >> >> [4]: >> >> https://github.com/theuni/ckpool/commit/7d84b1d76b39591cc1c1ef495ebec513 >> >> cb 19a08e >> > >> > I'm pretty sure this commit is actually /introducing/ a bug in working >> > (albeit ugly) code. The height, while always positive, is serialised as >> > a signed number, so 0x80 needs to be two bytes: 80 00. >> >> You're right, thanks. The current code breaks on heights of (for ex) >> 16513. I'll fix up my changes to take the sign bit into account. > > I'm curious what bug it was fixing? Was it overwriting data beyond the number? Using 16513 as an example: serialized by bitcoind: 0x028140 serialized by ckpool: 0x03814000 ckpool works because blocks after 32768 end up looking the same, but it will break again at 2113664. > > Luke ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bitcoin-dev] SegWit GBT updates 2016-02-02 1:40 ` Cory Fields @ 2016-02-02 2:30 ` Luke Dashjr 0 siblings, 0 replies; 7+ messages in thread From: Luke Dashjr @ 2016-02-02 2:30 UTC (permalink / raw) To: Cory Fields; +Cc: Bitcoin Dev On Tuesday, February 02, 2016 1:40:49 AM Cory Fields wrote: > On Mon, Feb 1, 2016 at 6:08 PM, Luke Dashjr <luke@dashjr•org> wrote: > > On Monday, February 01, 2016 9:43:33 PM Cory Fields wrote: > >> On Mon, Feb 1, 2016 at 2:46 PM, Luke Dashjr <luke@dashjr•org> wrote: > >> > Allowing for simpler cases both encourages the lazy case, and enables > >> > pools to require miners use it. It also complicates the server-side > >> > implementation somewhat, and could in some cases make it more > >> > vulnerable to DoS attacks. Keep in mind that GBT is not merely a > >> > bitcoind protocol, but is used between pool<->miner as well... For > >> > now, it makes sense to leave > >> > "default_witness_commitment" as a bitcoind-specific extension to > >> > encourage adoption, but it seems better to leave it out of the > >> > standard protocol. Let me know if this makes sense or if I'm > >> > overlooking something. > >> > >> I think that's a bit of a loaded answer. What's to keep a pool from > >> building its own commitment and requiring miners to use that? I don't > >> see how providing the known-working commitment for the > >> passed-in-hashes allows the pool/miner to do anything they couldn't > >> already, with the exception of skipping some complexity. Please don't > >> confuse encouraging with enabling. > > > > Making it simpler to do a centralised implementation than a decentralised > > one, is both enabling and encouraging. GBT has always been designed to > > make it difficult to do in a centralised manner. > > But your suggestion is "use libblkmaker" which will build the trees > for me. By that logic, isn't libblkmaker making a centralized > implementation easier? Shouldn't that usage be discouraged as well? libblkmaker is miner-side; right now it implies the miner using the templates as-is (perhaps after verifying the transactions meet some criteria), but it is the miner who is making that decision, not the pool. > And along those lines, shouldn't the fact that it's used as a pool <-> > miner protocol be discouraged rather than touted as a feature? ??? > >> What's the DoS vector here? > > > > It's more work for the pool to provide it, similar to the "midstate" > > field was with getwork. Someone performing a DoS needs to do less work > > to force the pool to do complex calculations (unless the same > > transaction tree / commitment is used for all miners, which would be an > > unfortunate limitation). > > It's being provided to them. And if they're using a modified set of > tx's, they'll need to re-calculate it in order to verify the result > anyway. I suspect I'm not understanding this argument. The DoS is against the pool, not the miner. You'd attack by pretending to be 100000 new miners per second, and the pool then needs to calculate a witness commitment for each one. It's a lot cheaper to just serialise and send the transaction list. > >> >> The issue in particular here is that a non-trivial burden is thrust > >> >> upon mining software, increasing the odds of bugs in the process. > >> > > >> > It can always use libblkmaker to handle the "heavy lifting"... In any > >> > case, the calculation for the commitment isn't significantly more than > >> > what it must already do for the stripped merkle tree. > >> > >> Agreed. However for the sake of initial adoption, it's much easier to > >> have a known-correct value to use. Even if it's just for the sake of > >> checking against. > > > > Sure, I'm not suggesting we remove this from bitcoind (probably the only > > place that makes initial adoption easier). > > How about exposing it as a feature/capability, then? That way pools > can expect it from bitcoind, but won't be required to expose it > downstream. Implementation-specific things aren't standards. And besides, they really *shouldn't* expect it from bitcoind; it's simply a reasonable compromise to provide it encourage adoption of SegWit. Once SegWit is live, there is no more value to doing so. Luke ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-02-02 2:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-30 18:50 [bitcoin-dev] SegWit GBT updates Luke Dashjr 2016-02-01 18:41 ` Cory Fields 2016-02-01 19:46 ` Luke Dashjr 2016-02-01 21:43 ` Cory Fields 2016-02-01 23:08 ` Luke Dashjr 2016-02-02 1:40 ` Cory Fields 2016-02-02 2:30 ` Luke Dashjr
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox