Goal: don't make service providers have to jump through near-impossible hoops to handle receiving generated payments/deposits (right now, only MtGox actually handles them correctly); this also makes things nicer for miners who receive to their own wallets (eg, they can tell which pool it came from)
ACK
I'm not sure if I understand it, do you intend to remove the "generated" status from the client? That would lead to more changes, I think of the mature vs. immature state and other strings used in Qt. Perhaps you can clarify this.
@Diapolo It seems to set it up so that generation transactions are just treated more like normal transactions and are tagged in the category "received". Since I would guess that anything that made use of this data already should be able to handle a "received" transaction, I don't see the issue.
Here is a paste of the last block I mined on testnet3 with it: http://pastebin.com/Q6HdDiiT
@wizkid057 I think there are still places in the code that need to be changed for the GUI client to handle generated like received, but I guess @luke-jr should give his thoughts on this, too.
It's not meant to make generation identical to ordinary received coins. That would create problems. It's just intended to make the common parts behave the same, so it's not a hassle to handle generated coins properly. With all current versions, it's impossible to tell what address or account generation was received on, which makes it impossible to handle them without hacking the code.
1581 | mapAccountBalances[strSentAccount] -= nFee;
1582 | BOOST_FOREACH(const PAIRTYPE(CTxDestination, int64)& s, listSent)
1583 | mapAccountBalances[strSentAccount] -= s.second;
1584 | if (wtx.GetDepthInMainChain() >= nMinDepth)
1585 | {
1586 | - mapAccountBalances[""] += nGeneratedMature;
Do I understand correctly that this doesn't change the "generated coins always to '' account" behaviour?
EDIT: nevermind
It does, intentionally.
I like this. Less special casing for generations and ability to assign them to accounts. I do think that changing the way for denoting generations (by "from" : "generation" instead of "category" : "generation") may need some discussion first though, as this breaks compatibility.
It is nice to remove this special casing
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/ee9eecb54db794a97de6e5add719aa6c4c020589 for binaries and test log.
ACK on the latest change ({"generated" : true} instead of {"from" : "generated"}).
One minor nitpick: setting the status to "processing" instead of "validating" only at 2 confirmations seems arbitrary to me. 1 confirmation seems more natural. Anyone else with an opinion?
171 | + if (!wtx.GetBlocksToMaturity()) 172 | + strStatus = "confirmed"; 173 | + else 174 | + strStatus = "processing"; 175 | + } 176 | + else
Why don't you just use else if, this looks weird ... same for all other occurrences!
I personally prefer to not use else if, but, that's me. Doesn't make the code any less functional.
Yes it is a personal favor, but I wanted to express my dislike here :-D. I think it's also harder to read.
The actual meaning/syntax is clearer this way, and plays nicer with diff
It is harder to see that the if()s belong to an else this way, while reading the code quickly, as they start on their own line just like stand-alone ifs. Also this is style is used nowhere else in the source.
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/22e16c5d56f9506f2ff7fbce01e488bc0551aaf4 for binaries and test log.
Can you rebase?
I don't like the "processing" state. I say simplify and go with:
"confirmed" -- 6+ confirmations (120+ for generation) "validating" -- 0+ confirmations ( 1+ for generation) "invalid" -- negative confirmations (contradicted in main chain)
Those three states plus the explicit 'confirmations' number should be plenty.
@gavinandresen I think it might be nice to have a state that represents "possible to spend immediately, but not confirmed yet" (so 1+/100+); thoughts?
I don't mind a "maturing"/"processing"/"confirming" state for 1+/100+, but I don't really care.
I think it might be nice to have a state that represents "possible to spend immediately, but not confirmed yet" (so 1+/100+); thoughts?
+1
Hm. I thought this was supposted to make generated txn work better with accounts, but it doesn't seem to. E.g. listtransactions "accountthathashadgenerationsenttoit" still doesn't give the expected results.
erp. Nevermind. I fail at testing— just didn't have an account set on my last p2pool address.
How about: "invalid" for conflicted txn / orphan coinbase "validating" for 0 "confirming" for 1-6 "maturing" for 1-120 coinbase "confirmed" for >=6 / >=120
In any case, shed painting the terms aside, this appears to work for me.
@gmaxwell Having a special status for coinbases contradicts the main goal of the status ;)
I'd worry "confirming" could be misread as "confirmed" too easily.
I can rebase this as soon as @gavinandresen makes a final call on the shed painting :)
Good call on confirming being potentially misread (even though the RPC is, in theory, just for machines— lots of code will pass along our text, and it costs nothing to use another string).
I think this is ready to go in, except for the rebase and shed painting.
Luke, it is long discussions like this where you're pig-headed and ignore what everybody else says that makes me want to ignore every single one of your pull requests.
So: fix your idiosyncratic coding conventions for ... else if.
And get rid of the status key entirely; applications have survived without it, and there is obviously no consensus on what words or confirmation levels aught to be.
And maybe this will make it into the 0.8 release. It is too late for 0.7.
Rebased without "status"
- Show address receiving the generation, and include it in the correct "account"
- Multiple entries in listtransactions output if the coinbase has multiple outputs to us
"category" changes reverted to be safer (without "status" it can more easily be misinterpreted).
This is a often requested big improvement, even without the status changes. With the category being left alone, it's reasonably conservative. I've tested it fairly extensively both on testnet and mainnet wallets.
Future pulls can put the status stuff back in after we figure it out in some future version. The status can also indicate things like observed doublespends. I'll open an issue for that.