< jonasschnelli>
Luke-Jr: Yes. We should backport the OP_NOP2 rename.
< morcos>
Luke-Jr: Can you point out where you think the priority calculation is broken (other than not including not in chain at time of acceptance inputs. I'm pretty sure it doesn't have an off by one error. but if so, it shoudl be fixed
< Luke-Jr>
morcos: immediately after an input is confirmed, its value is multiplied by 0 toward its dependent's priority, rather than 1
< Luke-Jr>
morcos: I discovered this while writing unit tests for your bugfix in 7149
< Luke-Jr>
although thinking about it today, I'm not entirely sure the fix needs to be as complicated as I made it
< morcos>
Luke-Jr: you're saying the supposedly correct dynamic priority is wrong, not that there is a "bug" in the calculation in master?
< Luke-Jr>
morcos: both
< morcos>
i'm sure that the calculation in 7149 is the same as the old calculaiton
< morcos>
its checked in mempool.check isn't it
< Luke-Jr>
morcos: for given height X, it is correct, but for block X+1, it uses height X
< morcos>
can you point me to a line # that you think gives a wrong result (in master) i'm having trouble following what you mean
< morcos>
but this was checked by comparing the new calculation to the old calculation and they were the same to within double precision
< Luke-Jr>
but I'm not sure even that fix is correct now
< Luke-Jr>
probably just the rpcblockchain and mining *calls* need a +1
< Luke-Jr>
oh, and AcceptToMemoryPool's usage probably ought to have a +1, but it hasn't worked right since it was added there
< morcos>
Luke-Jr: I htink you are mistaken
< Luke-Jr>
morcos: well, then explain why the unit test I wrote doesn't pass on it? :P
< Luke-Jr>
s/unit/rpc/
< morcos>
So if a new input is included in the block chain at block X. that input adds 0 to the priority of the tx at block X.
< morcos>
but now the inchainINput value will be increased by new input, and the cached height will = X
< morcos>
so if you ask for priority at (X+1) then it will include that newly inchain input in the aging
< morcos>
but this is only relevant to 7149 and not master anyway
< morcos>
Luke-Jr: which line in your test fails?
< Luke-Jr>
the main problem is you're asking for priority X when it should be X+1
< Luke-Jr>
morcos: literally every non-zero comparison
< Luke-Jr>
they're always off by one block
< Luke-Jr>
once block X is mined, the current priority for all practical purposes is X+1
< Luke-Jr>
because miners are mining X+1, not X
< morcos>
yes i agree, and that is what is evaluated for mempool acceptance and mining
< Luke-Jr>
mempool acceptance at least uses chainTip.height()
< morcos>
i'm not sure what is returned in getrawmempool, maybe that is wrong
< Luke-Jr>
mining appears to use the correct height, you're right
< morcos>
oh, interesting, perhaps there is a bug in the mempool acceptance code.
< morcos>
i dont think there is a bug in the calculation though.. i'll try to find some time today to look at it
< morcos>
can you point me to the branch with your rpc test so i can try it out
< Luke-Jr>
I haven't committed it yet, because the pruning rpc test fails with it, and I need to confirm it isn't because of my changes
< Luke-Jr>
it's in that patch I linked though
< morcos>
Luke-Jr: nope, no bug in ATMP
< morcos>
The dpriority that is calculated and stored as the starting priority is the priority of the tx at block X (chain tip)
< morcos>
but then when it is evaluated for acceptance to the mempool there is a separate call to GetPriority which feeds in chainActive.height + 1
< morcos>
see line 972
< morcos>
so if your concern is that startingriority represents a priority that is never actual used for anything, i suppose thats true, but thats the way its always been
< morcos>
i believe priority used in any calculations are always correct
< morcos>
the priority value returned by getrawmempool has always been incorrect for currentpriority anyway
< Luke-Jr>
[15:19:58] <morcos> The dpriority that is calculated and stored as the starting priority is the priority of the tx at block X (chain tip) <-- that's not what it ought to be
< Luke-Jr>
oh
< Luke-Jr>
I see
< Luke-Jr>
morcos: yes, seems the bugs are entirely in getrawmempool then
< Luke-Jr>
which as you note, has never worked right in that regard
< Luke-Jr>
still would be good to fix it though
< Luke-Jr>
if for no reason other than to have working rpc tests
< * Luke-Jr>
ponders adding priority to the GBT output so we can test that as well
< morcos>
i don't understand what you want to "fix"
< morcos>
startingpriority can be defined to be anything we want
< morcos>
its a meaningless number only used in getrawmempool rpc call
< morcos>
why change the definition of it?
< Luke-Jr>
"currentpriority" at the very least
< Luke-Jr>
with regard to startingpriority, why change it => so it has some meaningful purpose
< morcos>
by fix currentpriority you mean make it evaluate at X+1 ?
< Luke-Jr>
morcos: yes
< morcos>
ehh.. not worth it in my opinion.
< Luke-Jr>
…
< morcos>
jonasschnelli: ping?
< morcos>
your comments about NotifyTransaction on the abandontransaction PR
< morcos>
i'm concerned that that would be needed in MarkConflicted as well
< morcos>
I just copied the code from there.
< morcos>
I'm not very familiar with how the wallet/gui works, but NotifyTransaction is used to update the GUI I guess? What notifications do we have to worry about? Do other things like the available balance know to get updated if you just do that notification?
< morcos>
your other commits are also things that I think are good ideas, but I'm still holding out hope for abandontransaction to make it in 0.12, so not sure if it helps to add those now or not
< morcos>
again the tx categories are something I have no familiarity with, but it seems to me that you can mark txs you receive as abandoned as well (although i don't think this would have any practical effect now, it might if we in the future sort such txs differently)
< jonasschnelli>
morcos: still here?
< morcos>
yep
< * jonasschnelli>
reading backlog
< jonasschnelli>
morcos: I guess MarkConflicted() is always used together with the NotifyTransaction signal.
< jonasschnelli>
AddToWallet() calls the signal.
< jonasschnelli>
morcos: and sure, ... feel free to keep the informative PRs (listtransactions and new GUI tx state) out for 0.12.
< morcos>
jonasschnelli: oh. interesting.
< morcos>
thats what copy-paste coding gets you!
< jonasschnelli>
abandon use a own db write,... so there it's necesarry.
< morcos>
sorry, i didn't understand that?
< morcos>
but maybe you are trying to say what i was just going to ask about
< jonasschnelli>
hah...
< morcos>
it looks like markconflicted does things like write the tx to the database and mark it dirty on its own, when those same things happen in AddToWallet
< jonasschnelli>
Your new setAbandoned() call creates its own instance of CWalletDB()...
< jonasschnelli>
it's a rare case... because rest is adding keys or adding wtx.
< jonasschnelli>
in terms os wtx,.. everything goes over AddToWallet
< jonasschnelli>
not important at all, .. but you need to call the NotifyTransaction signal then
< morcos>
seems like i also might need to call the -walletnotify script
< jonasschnelli>
(which takes care for balance update, etc. in the GUI)
< jonasschnelli>
morcos: yes. Could be possible... also keep ZMQ in mind (maybe same interfae)
< jonasschnelli>
*interface
< morcos>
i think those same thigns are missing from MarkConflicted in the case it gets called from AddToWalletIfInvolvingMe right?
< morcos>
because it can cause txs to be marked as conflicted , but AddToWallet doesn't get called
< morcos>
because the tx we are considering is the double spend
< jonasschnelli>
yes... could be necessary there. Agreed. But out of scope four your abandoned PR?
< morcos>
perhaps, but i think a necessary bugfix for 0.12
< jonasschnelli>
Yes. Agreed.
< morcos>
ok.. i have to do interview soon, but will work on this this afternoon
< jonasschnelli>
-walletnotify needs also a refactor... your right.
< jonasschnelli>
But can be done later... happy interview.
< btcdrak>
Is there a way to prevent travis from running on a PR?
< Taek>
do verion numbers get updated when changes are backported?
< btcdrak>
Taek: what do you mean?
< btcdrak>
Taek: if something get backported to 0.10 for example it will eventually make it into a 0.10.x maintenance release, if that's what you mean.
< morcos>
jonasschnelli: wumpus: actually i took jonas's commit for #7312, but I'm not sure I understand well enough to know whether both additions he added are needed
< morcos>
I'm not sure how the GUI notifications work
< morcos>
but why would it be necessary to call NotifyTransactionChanged there for each of the txs that had an input being spent
< morcos>
But it isn't necessary to do that in SyncTransaction when we are marking those prevtxs dirty b/c their inputs are being spent in the first place?
< jonasschnelli>
morcos: I guess it's necessary.
< morcos>
jonasschnelli: it was hard for me to tell what that Notify call actually does
< morcos>
but it seems to me the two function are determine whether the balance needs to be updated and determine whethehr the transaction record should be shown
< jonasschnelli>
morcos: I think from the wallet perspective it's easy: notify every changed transaction.
< jonasschnelli>
What if a signal listener relays on a info when a child-tx gets abandoned?
< morcos>
jonasschnelli: its not done anywhere else that the only relevant update is whether the txs outputs are spent or not
< morcos>
for instance look at SyncTransaction
< morcos>
it marks all the prevouts as dirty so we can recalculate balances
< jonasschnelli>
(Phone typing)...
< morcos>
but it doesn't notify anything
< morcos>
well take a look tomorrow at your computer, but i think its unnecessary and isn't in keeping with the other places we call NotifyTransaction
< jonasschnelli>
Yes. Maybe we remove the second line. But I think we should look at it from a signal/future-feature perspective.
< morcos>
but I wasn't comfortable enough with the GUI code to change your commit
< morcos>
jonasschnelli: but we have to define what the signals are for
< jonasschnelli>
Okay. I'll have a closer look tmr.
< jonasschnelli>
morcos: not really. :-)
< morcos>
and if we want them to be for letting you know that some previously spent inputs are now unspent, then we need it to be the case for everytime that status changes
< jonasschnelli>
But IMO call whenever a transaction changes.
< morcos>
its very confusing to someone trying to understand the code, if sometimes you call it and sometimes yo udont
< morcos>
this would be literally the only case of calling it for a change like this i think
< jonasschnelli>
Yes. It should follow the same concept everywhere in the code. Agree.
< jonasschnelli>
Yes.
< jonasschnelli>
Remove the seconds line / signal call
< morcos>
ok, great, i'll wait for your thoughts tomorrow. i see your point. someone in the future might want to hide txs that have been completely spent or mark them differently or something.
< morcos>
but then we shoudl be consistent everywhere
< morcos>
ok
< jonasschnelli>
Right.
< morcos>
i'll remove it for now in the hopes that wumpus will merge this if its done changing
< morcos>
thanks!
< jonasschnelli>
Yes. We need to get this in asap.