Pages:

Author

Topic: Coinbaser branch's new JSON-RPC method - page 2. (Read 5993 times)

Luke-Jr

legendary

Activity: 2576

Merit: 1186

Quote from: shads on October 12, 2011, 09:27:50 AM

And those blocks without magic prefix are accepted by the rest of the nmc network? If so that's my concern dealt with.

Yes, they are.

Quote from: shads on October 12, 2011, 09:27:50 AM

*If* a reasonable limit on coinbase outputs could be added then for what it's worth I'd be happy to add my vote in favour on the change. Without said limits then my vote is in favour of the 'setworkaux' part of the change only.

And what reasonable limit would that be? There are at least legitimate circumstances for having tiny-amount outputs (I always include a single 1 Satoshi output so it shows up on my pool's listtransactions). I suppose there could be a hard-cap on total count of outputs, but what should that be? Maybe 1% of the current block size limit? That gives 294 outputs max at 1 MB (the current hard-coded block size limit)-- an average of ~0.17 BTC each.

shads

sr. member

Activity: 266

Merit: 254

Quote from: Luke-Jr on October 12, 2011, 08:31:38 AM

Quote from: shads on October 12, 2011, 05:33:31 AM

The remain concerns are: Does the -coinbase enforce either a minimum tx output value or a max number of tx outputs? If not it should. The primary use case for this for pools to payout their miners and have invalid blocks dealt with automagically. It there are no limits it's only a matter of time before some pools start paying out tiny balances to masses of miners. This could bloat the coinbase a lot unless I'm missing something.

There is nothing anyone can do to stop block-makers from producing tiny coins. Coinbaser does prevent invalid outputs, though-- if the output is not consistent (ie, tries to pay too much, to an invalid address, etc) it fails over the the default behaviour.

True anyone could create a monster coinbase transaction if they had the knowledge to construct a block from scratch and feed it into the network via p2p protocol. But if you knew enough to do that you'd probably know why it's a bad idea. A simple command line switch doesn't require any real knowledge of the network to use successfully and that's where the danger lies. A relatively technically unskilled pool op decides to payout every single miner on every won block. This is quite feasible and without any enforcement it just makes the problem accessible to people more likely to make the mistake.

Quote from: shads on October 12, 2011, 05:33:31 AM

My second concern is that Luke has made the following comment in the wiki:

Quote

Insert exactly one of these headers into the scriptSig of the coinbase on the parent chain. vinced's code always inserts an OP_2, 0xfa, 0xbe, 'm', 'm' in front of it, but you don't have to.

As far as I can see this patch doesn't deal with validation of the aux blocks. Presumably this will be left to vinced's original code as you'd require an update to all namecoin miners to alter it's validation rules. So the question is, will the absence of OP_2, 0xfa, 0xbe, 'm', 'm' before the aux header prevent vinced's validation from working? If so then the rpc call should check this and return an error if it doesn't.

No, the reason I changed this to say optional is because I am successfully using it on Eligius without that magic prefix.

And those blocks without magic prefix are accepted by the rest of the nmc network? If so that's my concern dealt with.

*If* a reasonable limit on coinbase outputs could be added then for what it's worth I'd be happy to add my vote in favour on the change. Without said limits then my vote is in favour of the 'setworkaux' part of the change only.

Luke-Jr

legendary

Activity: 2576

Merit: 1186

Quote from: shads on October 12, 2011, 05:33:31 AM

My second concern is that Luke has made the following comment in the wiki:

Quote

Insert exactly one of these headers into the scriptSig of the coinbase on the parent chain. vinced's code always inserts an OP_2, 0xfa, 0xbe, 'm', 'm' in front of it, but you don't have to.

No, the reason I changed this to say optional is because I am successfully using it on Eligius without that magic prefix. Furthermore, while the name "setworkaux" might be the source of confusion, it can be used for adding any arbitrary data to the coinbase, not just for the particular iteration of merged mining. There can very well be future functionality (like advertising your fee schedule, for example) using the coinbase as well.

shads

sr. member

Activity: 266

Merit: 254

I've only got two concerns left with this patch. Luke's done some good doco for both parts of it. And now that he's explained how you'd use a bit better I can see this becoming an eventual replacements for most of vinced's implementation. It is a much cleaner way in the long run.

The remain concerns are: Does the -coinbase enforce either a minimum tx output value or a max number of tx outputs? If not it should. The primary use case for this for pools to payout their miners and have invalid blocks dealt with automagically. It there are no limits it's only a matter of time before some pools start paying out tiny balances to masses of miners. This could bloat the coinbase a lot unless I'm missing something.

My second concern is that Luke has made the following comment in the wiki:

Quote

Insert exactly one of these headers into the scriptSig of the coinbase on the parent chain. vinced's code always inserts an OP_2, 0xfa, 0xbe, 'm', 'm' in front of it, but you don't have to.

Luke-Jr

legendary

Activity: 2576

Merit: 1186

Quote from: 2112 on October 10, 2011, 02:41:16 PM

Quote from: Luke-Jr on October 10, 2011, 02:25:41 PM

It is impossible and unreasonable to force everyone to have the exact same system clock.

I think that Lolcust and ArtForz provided a good counter-example by their GeistGeld alt-chain that uses SNTP for time synchronization. The objective is not "exact-same" system clock, but the system clock differences that are within currently acceptable norms: I think it is still one minute in the insurance industry and in something close to one second in the finance industry.

Notice that I didn't say it's impossible/unreasonable to actually have the "exact" (within reason) same clock. I said it's impossible/unreasonable to force it. But anyhow, it is indeed an off-topic discussion for this thread, which has nothing to do with block times. :p

2112

legendary

Activity: 2128

Merit: 1065

Quote from: Luke-Jr on October 10, 2011, 02:25:41 PM

It is impossible and unreasonable to force everyone to have the exact same system clock.

Luke-Jr

legendary

Activity: 2576

Merit: 1186

Quote from: 2112 on October 10, 2011, 02:13:19 PM

The argument that I have: it appears that "getmemorypool" is a step towards allowing the miner to record a reasonably accurate time of finding the block. Which is in turn a step towards more tight and accurate timekeeping in the whole block chain.

Your patch just perpetuates the existing bad situation where the block times are untrustworthy and acausal.

Block times will always be untrustworthy to a degree, and are already trustworthy to a degree. It is impossible and unreasonable to force everyone to have the exact same system clock. "getwork" does not enforce the time header, anyhow, so "getmemorypool" does not change this reality in any way. (that is, "getwork" allows miners to change the time at will)

2112

legendary

Activity: 2128

Merit: 1065

Quote from: Luke-Jr on October 10, 2011, 01:20:16 PM

What dissent? I see questions, and some "no" votes (strangely, most of which just appeared today), but no arguments against this.

Luke-Jr

legendary

Activity: 2576

Merit: 1186

Quote from: Gavin Andresen on October 10, 2011, 11:38:11 AM

Quote from: Luke-Jr on October 10, 2011, 11:33:05 AM

No, getmemorypool has nothing to do with setworkaux, coinbases, or merged mining. It's a JSON-RPC command that was merged with even less review than coinbaser, and is only used by a single pool.

Huh? getmemorypool discussion is/was here: https://bitcointalksearch.org/topic/proposal-new-rpc-call-to-get-the-transactions-to-be-included-in-the-next-block-39088

I'll just note that I'm damned if I do pull stuff that has some discussion here (and no objections raised), but damned if I don't pull stuff that first had no discussion, and then had a little discussion with some dissent.

1-2 people supporting getmemorypool vs 9 supporting coinbaser. hmm.

What dissent? I see questions, and some "no" votes (strangely, most of which just appeared today), but no arguments against this.

Gavin Andresen

legendary

Activity: 1652

Merit: 2216

Chief Scientist

Quote from: Luke-Jr on October 10, 2011, 11:33:05 AM

No, getmemorypool has nothing to do with setworkaux, coinbases, or merged mining. It's a JSON-RPC command that was merged with even less review than coinbaser, and is only used by a single pool.

Luke-Jr

legendary

Activity: 2576

Merit: 1186

Quote from: Gavin Andresen on October 10, 2011, 11:08:55 AM

Does setauxwork interact with the new getmemorypool RPC command at all? Should it?

No, getmemorypool has nothing to do with setworkaux, coinbases, or merged mining. It's a JSON-RPC command that was merged with even less review than coinbaser, and is only used by a single pool.

Quote from: Gavin Andresen on October 10, 2011, 11:08:55 AM

And should there be a listauxwork RPC command?

Possibly as a debugging tool.

Gavin Andresen

legendary

Activity: 1652

Merit: 2216

Chief Scientist

This is why I don't pull additions or major changes to RPC commands without at least a couple of people looking at them and either saying "yup, that's exactly how I would do it" or "it'd be better if...."

I agree with Mike, the -coinbaser should be separate from setauxwork.

Does setauxwork interact with the new getmemorypool RPC command at all? Should it?

And should there be a listauxwork RPC command?

Luke-Jr

legendary

Activity: 2576

Merit: 1186

Quote from: Mike Hearn on October 10, 2011, 10:03:18 AM

setworkaux looks pretty much like what I'd imagined. You might want to split the command line option out, as that's a separate and unrelated change.

They're very much the same change: they both together support customization of the coinbase transaction.

Quote from: Mike Hearn on October 10, 2011, 10:03:18 AM

BTW, what's the backwards-time detection stuff about? I know time can sometimes go backwards on some machines, but you could add a comment explaining why this code is there (was it observed in the wild on eligius machines)? Also it doesn't currently log anything when extraNonce overflows - is that an exceptional condition or something that's expected to happen normally? If the former (it's a bug) maybe assert/terminate. If the latter does it need to be logged? If it's unknown, logging for a while seems to make sense.

Just a safety precaution. I don't know if it's possible or ever occurs. (otoh, it's very easy to see many existing code paths in bitcoind cannot possibly occur... Wink

)

Mike Hearn

legendary

Activity: 1526

Merit: 1129

setworkaux looks pretty much like what I'd imagined. You might want to split the command line option out, as that's a separate and unrelated change.

BTW, what's the backwards-time detection stuff about? I know time can sometimes go backwards on some machines, but you could add a comment explaining why this code is there (was it observed in the wild on eligius machines)? Also it doesn't currently log anything when extraNonce overflows - is that an exceptional condition or something that's expected to happen normally? If the former (it's a bug) maybe assert/terminate. If the latter does it need to be logged? If it's unknown, logging for a while seems to make sense.

Luke-Jr

legendary

Activity: 2576

Merit: 1186

Quote from: Mike Hearn on October 10, 2011, 05:32:33 AM

I'd really like to see something like this get in, but I agree with Steve - it's not clear why this is different to what Vince has already done.

Vince's stuff is a mess of unreadable code, that requires putting a proxy (additional point of failure/bottleneck) in front of bitcoind. This approach is much simpler (at least insofar as modifications to bitcoind) and has no effect on the stability of the primary Bitcoin mining operation (since the merged-mining manager runs parallel to bitcoind, not in front of it). These changes are also more abstract, not necessarily tied to merged-mining specifically.

Mike Hearn

legendary

Activity: 1526

Merit: 1129

I'd really like to see something like this get in, but I agree with Steve - it's not clear why this is different to what Vince has already done.

Luke-Jr

legendary

Activity: 2576

Merit: 1186

Gavin has apparently decided (for no reason) that he isn't going to merge this, even though this thread met his requirement of having "community support" for the new "setworkaux" JSON-RPC method. I doubt I'll bother maintaining this through the next version, so I guess pester Gavin if you want it (or hope it gets merged to post-0.5 git before it needs yet another rebase...)

shads

sr. member

Activity: 266

Merit: 254

I'm not going to vote until I understand it better.

I think it replicates some of the functions of getworkaux from vinced's merged-mining fork. If so there should be some discussion of which to use before considering either for pulling into the main branch.

Given that merged-mining-proxy uses getworkaux along with a couple of other new rpc calls, and that merged-mining-proxy will most likely be the fallback option for pools using pushpool to become merged-mining capable... and that I'm currently implementing merged mining into poolserverj using these calls from vinced's fork it would seem likely that it's going to gain wider adoption.

I'm totally open to using your method if it's somehow an improvement on the vinced method but simply saying it allows you insert data into the coinabse doesn't really explain the workflow of constructing a merged mining block. You still need a way to get the header hash of each aux chain block (getwork and hash yrself?), contruct a merkle tree then presumably pass the merkle root of this to setworkaux. I can figure it out that far but how under your scheme do you submit a solution to an aux chain?

Luke-Jr

legendary

Activity: 2576

Merit: 1186

Quote from: Luke-Jr on October 05, 2011, 02:19:18 PM

Quote from: Gavin Andresen on October 05, 2011, 02:11:22 PM

I think setworkaux is OK, but I don't like "doesn't work on windows" changes to support one mining pool.

It might work on Windows with a few changes, but I don't have a Windows system to test on. http://msdn.microsoft.com/en-us/library/96ayss4b(v=vs.71).aspx

Ok, ported coinbaser to Windows. Surprisingly, the problem wasn't popen, but fdopen (for TCP support).

Luke-Jr

legendary

Activity: 2576

Merit: 1186

Quote from: simplecoin on October 05, 2011, 05:30:42 PM

So, how do you intend setworkaux to be used?

From the code:

Code:

setworkaux  [data]
If [data] is not specified, deletes aux.