< BlueMatt>
or, well, easy to review for anyone who can read a list of variable names and trace their usage :p
< jtimon>
yeah, I wish I could review that, but never paid much attention to the network code above processMessages and I wasn't able to keep up with the recent changes (I still greatly appreciate the separation and look forward studying it better after all these improvements)
< BlueMatt>
well #9535 is just a lock split, so just going through each variable that is accessed in one side and making sure its not accessed on the other is a pretty good (though admittedly not 100% sufficient) review
< BlueMatt>
and one one of the sides in this case there are relatively few variables accessed, so its not so hard
< jtimon>
I just meant the code changes are small
< jtimon>
I guess I could do a mechanical review like what you propose
< cfields>
BlueMatt: yep, will do after dinner
< cfields>
jtimon: yea, it's sane, just needs manual review of what's accessed under the locks before/after
< cfields>
(note that you can treat it as non-recursive, so very simple)
< jtimon>
yeah, I considered it as the network-related PR I was more likely to be able to review, but then I tried to estimate how much would it take to do that and my answer was "no idea, let's move to another PR for now", if you say it's easy, thanks, I'll gladly do that (maybe not today, was hoping to at least utACK some individual commits in #9512)
< cfields>
jtimon: np. no obligation, was just echoing what matt said
< jtimon>
cfields: sure, it's a good tip, if you guys tell me it shouldn't take long to do that manual review I think it's a good use of my time, hey, it's dividing an important lock into 2! I guess I cs_main got me too scared about reviewing concurrency unless it was cs_args, cs_mempool (encapsulated in its class), or maybe even a 10-line singleton for libsecp's context or something
< jtimon>
about a previous version of it gmaxwell once said "This adds news without frees", and I believe I removed all the non-frees many rebases ago...
< jtimon>
also, how should I benchmark #8498 to make it attractive?
< gribble>
https://github.com/bitcoin/bitcoin/issues/8498 | Optimization: Minimize the number of times it is checked that no money... by jtimon · Pull Request #8498 · bitcoin/bitcoin · GitHub
< BlueMatt>
jtimon: do a few reindexes with and without it on a relatively unused machine
< jtimon>
BlueMatt: that will show some improvement (since connectblock is still calculating the tx fee twice), but the most interesting effect should be with acceptToMemoryPool...oh,wait, now that's only calculating it twice per tx too (it used to be much more), yeah, thanks, can try that (probably after 0.14)
< jtimon>
should probably have a look at src/bench and maybe try to do the reindex from there
< BlueMatt>
luke-jr: re: FlexVer...talos failed to get the (crazy high) funding they were trying to raise, so thats not gonna happen
< luke-jr>
BlueMatt: no, FlexVer survives beyond Talos
< luke-jr>
without more funding
< BlueMatt>
oh? are they putting it in something else?
< BlueMatt>
(my understanding was the raptor folks were essentially saying "ok, well we didnt get the funding, so if you want any pieces of talos you have to come pay us for them")
< luke-jr>
non-workstation/servers
< BlueMatt>
well, ok, technically they have another 48 hours to raise the missing 3.something million USD.....
< luke-jr>
FlexVer-based VPS is their fallback after Talos failing
< midnightmagic>
that funding target was pretty nuts. :-(
< BlueMatt>
oh? link?
< BlueMatt>
midnightmagic: yea...real shame, too
< BlueMatt>
they should have done the shit the ORWL folks did - raise a modest amount and assume you can sell more of the hardware later to make up the missing costs
< BlueMatt>
of course its a big risk, but at least we'd get cool stuff out of it :)
< midnightmagic>
Perhaps they just didn't have the money to do it to begin with.
< midnightmagic>
Similar failures have happened many times in the past e.g. phase5's attempt to resurrect the Amiga with the A\Box
< BlueMatt>
ooo, didnt know about that
< * sipa>
googles
< gmaxwell>
BlueMatt: I dunno, maybe, will have to think; but if you're concerned about security here there are many better things to do that would harden it that wouldn't change performance.
< gmaxwell>
BlueMatt: e.g. make it so that it can't skip signatures in a reorg.
< cfields>
BlueMatt: thanks for fixing up the const. I think my gripe wasn't really justified. The const rules for pointers/references still trip me up sometimes.
< BlueMatt>
gmaxwell: hadnt thought about that one...came up with a few things which addressed the specific scenario I described above, though figured that would be very scenario-specific :/
< BlueMatt>
cfields: I agree it was super hard to read what was going on there, and why it wasnt some crazy const_cast unsafe bullshit the way it read
< gmaxwell>
BlueMatt: another one is to not skip signatures when there is a long fork that doesn't cointain this block.
< gmaxwell>
(both of these were security features I recommended for the hashpower only decision-- but I didn't think their complexity was worthwhile with a static hash.)
< cfields>
BlueMatt: right, that. const_cast usually just sets off alarms to me that something's fishy. The change makes it easy to reason about.
< BlueMatt>
gmaxwell: yea, I wanted to avoid suggesting adding twenty ways it could be hardened and risk it slipping for 0.14
< gmaxwell>
I think both are more powerful than just burriedness, I even only did the burriedness because it was ~1 LOC.
< BlueMatt>
gmaxwell: both assume you actually see the other branch, which isnt a crazy assumption, but is different from burriedness
< BlueMatt>
well, reorg less so
< BlueMatt>
but either way, I'm happy if 2 weeks meets your performance goals
< BlueMatt>
if we come up with fun ways to strengthen it in the future (eg when jl2012's fork warning stuff gets fixed it would be easy to combine that), then we can do that
< luke-jr>
sipa: essentially DRM that allows selling encrypted VPSs that the physical-access/datacentre/host-manager cannot see into or control (beyond poweroff/delete)
< cfields>
jonasschnelli: I'm going through #9294, though I'm afraid I don't know the code well enough to give any helpful review. So nits is the best I can do :(
< gmaxwell>
because in that case you'd be on the attack chain, which is best header, but there would be a long valid fork that doesn't contain your block.
< cfields>
sipa: am I going to throw you off if I go ahead and squash 9441? Not sure if you want to have another look after the last few cleanups
< BlueMatt>
gmaxwell: ok, so do you want to add that? I do not think it is a replacement for setting the time to 2 weeks, plus setting the time to 2 weeks avoids the need to get re-reviews when we're already not gonna make our goals
< gmaxwell>
14:52 < jtimon> regarding the 5.5 vs 7.5 h benchmark, that's only for fresh releases or people setting the arg manually right before the limit, right?
< gmaxwell>
14:52 < BlueMatt> yes
< gmaxwell>
no increasing the offset increases the total sync time for that software forever.
< BlueMatt>
huh?
< BlueMatt>
did i misread the code?
< gmaxwell>
oh no I'm being momentarily stupid.
< BlueMatt>
the software is updated with some assumevalid....at that time all blocks a week before that assumevalid arent validated...one week later all blocks up to that assumevalid are validated
< BlueMatt>
ok, phew
< jtimon>
huh? +1 (I bet the answer is in GetBlockProofEquivalentTime which is the part I didn't fully read because it wasn't being changed in that PR)
< gmaxwell>
BlueMatt: I'm referring to the case where the user updates it. Which I expect people to do on low power devices.
< BlueMatt>
ahh, yes, ok
< gmaxwell>
it basically says you can't sync without processing N-back no matter what you set. So if you're on a low power device where a month of blocks takes hours, thats what you're left with.
< BlueMatt>
how long does it take to sync a week's blocks on an rpi?
< BlueMatt>
or, cdecker you were complaining about stuff on the cheapest digitalocean thing
< BlueMatt>
how slow was that?
< jtimon>
but the user doesn't have to do anything right? just wait for the program to fully the N last blocks, right?
< jtimon>
s/fully the/fully verify the/
< gmaxwell>
BlueMatt: a lot of people just copy the chain from another system; instead they could copy the assume block.
< BlueMatt>
on the flip side, a lot of setup guides will tell you to copy blockchain.info's latest hash as the assumeblock the day after release :p
< gmaxwell>
I also worry that making this too slow increases the occurance of people copying the state. E.g. there is someone on reddit that responds to every thread about slow sync with a download for a synced node.
< BlueMatt>
oh, agreed, but there is a line to walk there
< gmaxwell>
BlueMatt: thats better than taking a whole download of the state which people are already being told to do.
< jtimon>
re N blocks vs time: I definitely like height more, for everything related to time in consensus
< BlueMatt>
(I'd tend to agree that a week is sufficient to keep stupidity of random block explorers from creeping into consensus)
< BlueMatt>
just prefer > 1 week for other reasons
< BlueMatt>
I'm open to being convinced if it really is prohibitively slow (compared to the rest of chainsync w/o sig-checks)
< jtimon>
To reiterate, I don't see the danger with doing a month either: advanced users that look for top performance can compile the code changing the month to a week or 2 blocks
< BlueMatt>
I dont think thats a realistic request
< gmaxwell>
jtimon: none of this should involve time or height.
< gmaxwell>
__sigh__ both are trivially controlled by miners.
< gmaxwell>
the test in the code involves work.
< jtimon>
I guess my point is that "I want to resync faster than the assumevalid by default in the last release" use case is not very compeling
< gmaxwell>
jtimon: it is very compelling when it's taking days to sync and can be fixed.
< jtimon>
gmaxwell: the week sounds like time, but sorry, I admited I don't fully understand GetBlockProofEquivalentTime so I shouldn't have made the time vs height comment and I bet you're right it was irrelevant
< BlueMatt>
uhhh, wut....just fetched from github and compared my local branch between my two machines as always....except this time they're different
< MarcoFalke>
huh?
< MarcoFalke>
same commit hashes?
< BlueMatt>
different commit hash, same tree
< * BlueMatt>
goes to read git-log to see if github is fucking with shit or if I'm confused
< BlueMatt>
oh, nvm, false alarm
< * BlueMatt>
was testing git commit --amen and forgot that i changed the commithash on the top commit
< cfields>
BlueMatt: I suppose you just added cs_sendProcessing for general correctness? afaik, with the current single-threaded processing, there's no actual need for it?
< BlueMatt>
"Technically cs_sendProcessing is entirely useless now because it
< BlueMatt>
is only ever taken on the one MessageHandler thread, but because
< BlueMatt>
there may be multiple of those in the future, it is left in place"
< BlueMatt>
but, yea, its for correctness
< cfields>
haha
< cfields>
at least we agree :)
< BlueMatt>
true
< cfields>
serves me right for not reading. thanks.
< cfields>
it'd be nice if github emphasized the display of full individual commit messages better. I'm well aware that I put much more effort into writing them than reviewers put into reading them. And I'm equally guilty in my own reviews.
< * BlueMatt>
generally tries to avoid reading too much author-written text during review
< BlueMatt>
too easy to see what the author intended instead of what it actually does
< BlueMatt>
I tend to read the commit message if I'm confused to see if it helps me understand, but only if I have an idea that it might be broken
< cfields>
fair point
< BlueMatt>
ok, I'm down to 4 prs to review tomorrow...should be fun
< cfields>
heh
< cfields>
feeling better, i hope?
< BlueMatt>
yea, well enough to review, at least
< BlueMatt>
taking a day off to lie in bed helped a ton
< cfields>
good
< cfields>
those days are helpful even when you're not sick :)
< cfields>
maybe not in bed all day, but disconnected enough to make you crave code again
< BlueMatt>
oh, didnt mean to imply I didnt have email to respond to all day :p
< fanquake>
Has anyone running master noticed sync/reindex stalling just prior to getting a connection refused message in the debug.log?
< fanquake>
Have just had one instance of the GUI freezing/lagging, and no debug output for ~10 seconds. Then a few UpdateTip messages followed by a Connection refused.
< fanquake>
Testing 9484 btw, I think I'll get a full sync in just less than 2 hours.
< fanquake>
1hr40m to block 424'000
< gmaxwell>
fanquake: my first guess is the connection refused is just that the sleep that drove it hit its timeout while the slow operation happened, so it happened immediately after update tip.
< gmaxwell>
fanquake: and I would assume the updatetip took a while whole holding CS main because it was processing a bunch of blocks at once due to a reodering.
< gmaxwell>
e.g. you get a bunch of blocks with one massively out of order, when you finally fill in that missing block it goes and connects 100 at a time holding locks the whole time, which stalls the gui and causes some loss of network transfer speed.
< gmaxwell>
fanquake: there may be some more interesting bug lurking for sure... but if so I wouldn't notice it because I'd discount it because we already know that ProcessNewBlock can have very high latency.
< fanquake>
gmaxwell thanks for the explanation. I'll collect a few more details and open an issue to track it.
< fanquake>
gmaxwell btw, final sync time with 9484 -dbcache=4096 -par=8 was 2h12m
< fanquake>
s/sync/-reindex-chainstate
< gmaxwell>
you might want to add some logs around proceessnew block, e.g. enter and exit times. to try to validate my theory.
< gmaxwell>
fanquake: fantastic.
< fanquake>
That was to block 447885.
< gmaxwell>
we could also consider adding a locking debug mode that saves the time of lock/unlock and prints if a lock is held more than (say) 1 second by a single piece of code.
< cjamthagen>
Are timelocked transactions included in your mempool if they are not yet valid?
< morcos>
cjamthagen: no, they are not
< morcos>
luke-jr: just want to be sure you realize in #9519 it's not all txs that signal opt-in RBF that are excluded, but only actual replacements, regardless of whether they themselves signal opt-in RBF.
< jeremyru1in>
This is not really critical, but as jtimon points out there is a lot of confusion over "Trivial" tags for PR's. https://github.com/bitcoin/bitcoin/blob/03dd707dc027fbf6f24120213f8eb66571600374/CONTRIBUTING.md is the closest thing to a specification of what Trivial means. I think that we'd save ourselves a lot of confusion if we adopted a policy that was more in line with how people typically (and in many other projects) use "trivial
< jeremyru1in>
oops
< jeremyru1in>
I see jnewbery has just opened something on that, will move my message there
< achow101>
is the sync overlay thing supposed to show up on windows? because I don't see it
< gmaxwell>
cjamthagen: only if they'll be valid at the next block or sooner.
< sipa>
jonasschnelli: yes, that patch fixes the lack of dns response
< jonasschnelli>
sipa: Great.
< instagibbs>
can I PR directly against a BIP or should I bug the author?
< jtimon>
instagibbs: not sure what's the norm, but you definitely can technically, I received some PRs to my PR for bip99
< jonasschnelli>
instagibbs: I think you can PR but don't expect a merge without authors ack
< jtimon>
oh, you mean PR directly to the repo, not his own PR
< jtimon>
yeah then what jonasschnelli said
< sdaftuar>
BlueMatt: not sure you saw my comment in #9375 -- i suggested adding a "to do" comment in ProcessGetData, to remind us of the issues we discussed in requests for invalid blocks. thoughts?
< luke-jr>
morcos: oh, I didn't realise that. But we can't know if they are or aren't replacements necessarily?
< instagibbs>
RIP github
< BlueMatt>
annnddd github down
< instagibbs>
anyone need coffee
< BlueMatt>
yes!
< paveljanik>
good idea!
< BlueMatt>
sdaftuar: hum, I did miss that comment
< BlueMatt>
sdaftuar: remind me of the context again?
< * luke-jr>
just got hot cocoa
< BlueMatt>
for those bored during the github outage, https://0bin.net/paste/gPzFQpbXtDj9hQO8#NG+mcjz5Xuro4XPK4ZijCtV0sE4U-nFdoi8AWBL1IZX is #9535 and while its not tagged 0.14, its itself a big win and it would be swell if it could go in
< sdaftuar>
BlueMatt: general issue is that we don't have a way to tell a peer that we're intentionally ignoring a request, so our peer can't distinguish stalling from "sorry i now think this block is invalid"
< sdaftuar>
i think we talked about eventually extending the p2p protocol to communicate this, potentially?
< sdaftuar>
but documenting that we might announce a block and then ignore the request in ProcessGetData() seems prudent
< BlueMatt>
I'm not sure we talked about it, but, yea, thats something to consider...
< luke-jr>
if github is down a day, do we defer the freeze a day too? :P
< BlueMatt>
we do, technically, have reject messages, but ignore them because they're already not serialized with the rest of the p2p protocol
< BlueMatt>
luke-jr: I vote yes (and will do review today either way :p)
< sdaftuar>
right, we could extend the reject message generation to also apply to getdata messages
< sdaftuar>
rather than just block/tx messages
< luke-jr>
I wish GitHub's review thing worked offline
< achow101>
it looks like they're back
< BlueMatt>
sdaftuar: we'd probably have to also fix the reject messages so that they are serialized
< BlueMatt>
instead of at some random time in the future, who knows
< morcos>
luke-jr: yes we do know that... look at the code when GitHub is back
< sipa>
going over 9441 a last time
< BlueMatt>
sdaftuar: actually, if we're gonna extend it, we should just throw out the reject messages we have now entirely (no one uses them, they have some simple design flaws, and they are a big anonymity issue)
< BlueMatt>
sdaftuar: then we can add something that says "I will not respond to your block request"
< BlueMatt>
but we will send such a message only if we already announced said block
< BlueMatt>
(and this would not apply to transactions)
< BlueMatt>
someone wanna kick travis for #9484? sipa or wumpus, though I think gmaxwell can do it too
< jeremyru1in>
Ugh that's annoying that it reports that here ^^; just did that to force travis to restart Z(failed during the github downtime due to not being able to hit github)
< luke-jr>
jonasschnelli: would it help if I go over my suggestions for 9294 and do them myself?
< sipa>
BlueMatt: restart
< gmaxwell>
jeremyru1in: force push also does that.
< jeremyru1in>
nooo matt now nothing will get merged
< BlueMatt>
sipa: I'm too lazy to squash #9375 (and each commit passes the "it works, and passes test" rule), fyi, in case you were planning on waiting for that
< jeremyrubin>
gmaxwell: yeah, I was just lazy and on github.com at the moment :/
< cfields>
\o/
< cfields>
BlueMatt: 9535 needs rebase
< BlueMatt>
cfields: done
< cfields>
thanks
< sipa>
ugh, master broken in p2p-segwit?
< * sipa>
restarts
< sipa>
i tested locally before merge
< bitcoin-git>
[bitcoin] practicalswift opened pull request #9544: Add end of namespace comments. Improve consistency for anonymous namespaces. (master...consistent-use-of-end-of-namespace-comments) https://github.com/bitcoin/bitcoin/pull/9544
< bitcoin-git>
[bitcoin] practicalswift opened pull request #9545: Add override:s where appropriate (master...add-overrides-where-appropriate) https://github.com/bitcoin/bitcoin/pull/9545
< cfields>
sipa: pebkac, i hope?
< sipa>
cfields: ?
< cfields>
<sipa> ugh, master broken in p2p-segwit?
< sipa>
i assume it's somehow a spurious travis error
< sipa>
not pebcak
< cfields>
oh, i didn't see the failure, thought you were seeing it locally. got a link?
< sipa>
though i think the log may be unavailable as i've restarted
< BlueMatt>
grrr, I hate reviewing wallet changes
< gmaxwell>
BlueMatt: why? it use to be more obnoxious but I think it's no big deal now.
< BlueMatt>
because unlike some of our other codepaths, any change to eg spendability (like bumpfee) means reviewing like 20 functions which use spendability in slightly different ways
< morcos>
BlueMatt: I think its useful to look at it similar to how sdaftuar suggested. imagine you stop your node, you restart it, and before your wallet can try to reaccept your old transaction an external replacement comes in. its not required to _do anything_ to affect whether your orig tx affects spendability.
< morcos>
All bumpfee is doing is adding an extra layer of conservativeness on top... You know what... if you replace a tx, you're never going to be allowed to chain unconfirmed txs on the original tx again..
< BlueMatt>
morcos, sdaftuar: I'm not convinced that not allowing a user to spend an input they removed from a replacement is The Right Behavior, especially as we move towards more loose replacement policies, but I'm fine with that for now. I don't see how thats massively different from an abandon (which means "pretend this isnt here, unless it somehow gets confirmed")
< gmaxwell>
BlueMatt: bumpfee doesn't change the inputs. And any released inputs should not be spendable until the other spend is clearly conflicted.
< morcos>
I think we're conflating two different things here... Whether you can chain transactions off the original tx (the only change made in bumpfee) and whether any inputs spent by the original tx are still considered spent by the original tx (this is treated the same way any double spend is, both spends are spends)
< BlueMatt>
gmaxwell: yes, this was essentially all theoretical as it cant happen yet
< gmaxwell>
since bumpfee can't do that right now we can ignore having to handle the case where it does become spendable yet..
< morcos>
If you want inputs spent by the original tx to not count as spent, there is nothing stopping you from manually abandoning the original tx, but i can't possibly imagine thinking it was a good idea to happen in an automatic fashion
< gmaxwell>
morcos: well it should abandon the replacement or replaced txn automatically once it is well conflicted, I think? (doesn't need to now, but eventual functionality)
< BlueMatt>
morcos: I think eventually if we have some super fancy gui that allows you to replace a transaction, and you swap the inputs selected, it woud be weird to not be able to re-spend them, as you can for abandon
< BlueMatt>
but, agreed, lets table for now
< BlueMatt>
the thing it does now is the more conservative option
< BlueMatt>
which is good, and its a moot point unless we have other features added
< gmaxwell>
BlueMatt: are you saying you should be immediately able to respend them in that case?
< BlueMatt>
gmaxwell: dunno, probably not? unclear to me...but, lets table for now
< gmaxwell>
BlueMatt: if so that would be a huge footgun, I think, as it would assume the user has a better mental model for what gets confirmed than we know they do. (than even many of us do much of the time).
< BlueMatt>
but should be able to respend them after the replacement confirms, which you cannot do now
< BlueMatt>
gmaxwell: yea, well abandon is the same thing :p
< morcos>
gmaxwell: but that happens automatically right?
< BlueMatt>
morcos: my reading of the code now, you cannot ever respend the inputs which are no longer being replaced
< morcos>
if A spends inputs 1 , 2, 3 and you use futurebumpfee to replace A with A' which spends inputs 2, 3, 4
< sdaftuar>
yeah when the replacement confirms, the original will be cobnflicted, freeing up the inputs
< morcos>
then until either are confirmed, 1,2,3, and 4 will appear spent. When A' confirms, input 1 will automatically become spendable again because A is conflicted.
< morcos>
this seems like roughly the right behavior, and more importantly not a change from the existing behavior
< morcos>
but maybe BlueMatt is saying there is a bug and thats not happening?
< BlueMatt>
sdaftuar: oh, youre right, sorry missed that
< morcos>
BlueMatt: I'm not sure I agree with that either...
< morcos>
In the case A
< morcos>
sorry
< morcos>
In the case A' becomes conflicted... you want to Relay A, so I think that means you always want to relay A
< morcos>
it would be nice if you first try to reaccept everything and then only relay them if they are in your mempool at the end
< gmaxwell>
we shouldn't be relaying multiple verions of conflicting transactions at a time. but the mempool will make sure we don't.
< morcos>
I _think_ that'll be mostly what happens in practice, but i guess its not guaranteed.. (so assuming you have both A and A' probably only the second will be relayeD)
< BlueMatt>
lets say you have A and its been replaced by A' -> normally you restart, A (might be) accepted first, then A' replaces it, like normal
< BlueMatt>
I think this does mean we'll announce A, but proobably not if its been delayed-announced
< BlueMatt>
however, if the user restarts with a higher min relay fee, A' might not meet the replacement requirements
< BlueMatt>
and so A ends up in your mempool
< BlueMatt>
which sucks
< morcos>
isn't it always delayed-announced
< BlueMatt>
dont think so?
< sdaftuar>
BlueMatt: i agree that is a possible edge case
< morcos>
i mean you're almost right.. A' by definition has a higher relay fee
< morcos>
but you could restart with a higher dust limit, which could cause A to be accepted and not A'
< BlueMatt>
morcos: huh? relay fee is a property of your software, not the transacion?
< morcos>
but in that case you probably want A to be relayed!
< BlueMatt>
yes, dust limit is calculated from minrelayfee, iirc
< gmaxwell>
we probably should sort the unconfirmed transaction by dependency/relay fee before inserting, instead of just by order. but I don't think thats urgent now.
< morcos>
A' by definition has a higher feerate
< morcos>
gmaxwell: agreed.. not urgent... this is like rare edge case
< morcos>
and even if both get relayed or the wrong one, its not necessarily a BAD thing
< BlueMatt>
I think its an easy solution to just add a replaced_by check prior to relay?
< morcos>
but that has downsides
< sdaftuar>
that opens the door to morcos' edge case
< BlueMatt>
ehh...I mean I think once you've replaced you probably want the replacement relayed
< BlueMatt>
like, if you restart and it has a lower feerate that gets stuck in your mempool, that sucks and just means you're gonna have to replace again
< BlueMatt>
plus you might've done something else in the replace, like set it to not-replaceable
< BlueMatt>
or whatever
< morcos>
but the only reason the old one is stuck is there is something wrong wiht the new one
< morcos>
in that case you are basically just screwed anyway, unless you replace the new one
< BlueMatt>
not really...? the new one is just as fine as the old one
< morcos>
then why is that not replacing the old one in your mempool?
< BlueMatt>
just maybe that the new one wont auto-replace the old one in peoples' mempool across the network
< morcos>
don't you have chicken soup to eat?
< BlueMatt>
heh
< morcos>
oh b/c of the incremental relay fee getting raised
< morcos>
i missed that that was your point
< BlueMatt>
because either a) you're manually setting a higher -minrelayfee (for some reason...)...or b) you updated to a new version with a higher -minrelayfee default
< BlueMatt>
oh, sorrry, yes, that was my point
< BlueMatt>
so maybe the replacement wont relay that well depending on how much the rest of the network has upgraded
< morcos>
sure.. but look.. you shouldn't NEED to rerelay either of them
< morcos>
just b/c you restarted your node, doesn't mean everyone else did
< BlueMatt>
huh???
< BlueMatt>
and if you're (bumped) fee is low enough that it'll take a few days to get in?
< BlueMatt>
we cant break re-announce???
< BlueMatt>
eg a key use-case for bumpfee is setting the fee super low and bumping it eventually if it doesnt get in for a few days
< morcos>
we're not breaking it.. we're just saying there is a contrived edge case where it might not be super effective
< BlueMatt>
or a week
< BlueMatt>
whats the downside of adding the check to relay???
< BlueMatt>
it fixes an edge case, and....?
< BlueMatt>
eg I use bumpfee in greenaddress to send transactions with super low feerate to transfer between my wallets cause I dont care about conf time
< sdaftuar>
if you have upgraded settings so that the new tx is not sufficiently above the old one's feerate to relay through your own node, you might as well assume that you have to fee bump again to get it to relay across the network
< morcos>
i think the right way to do it is what gmaxwell said... some sort of sort.. where you follow the chain of A->A'->A'' to the end and then start by trying to rebroadcast A''
< BlueMatt>
and if it never gets in, i just bump it slightly
< BlueMatt>
sdaftuar: nope, it could time out, or maybe you're on a new version that much of the network isnt
< morcos>
but thats an improvement that can be done later
< BlueMatt>
(or you manually set -minrelayfee higher, which a bunch of folks did when we didnt have limited mempool and likely still have)
< morcos>
just blindly saying skip A if we have A' is not obviously better to me
< sdaftuar>
what would you have done if you upgraded before the bumpfee call?
< BlueMatt>
morcos: so you announce all three txn potentially to your peers? that blows
< sdaftuar>
BlueMatt: that is already what your recipient(s) will do
< morcos>
how so... relay happens to run in the middle of reacceptWalletTransactions
< BlueMatt>
yea, also what about announce? I'm pretty sure there are a few not-delayed-announce cases?
< morcos>
ok, how about this for a fix
< morcos>
its hacky but i think strictly better
< BlueMatt>
(also because we're likely to change relay in the future to do some fast-path'ing stuff through some nodes)
< morcos>
on startup only, we call ReaccpetWalletTransactions with a bool which skips txs that have been replaced
< * BlueMatt>
is still super confused as to why y'all want to try to add a replaced transaction to mempool
< gmaxwell>
BlueMatt: maybe the replacement is non-standard?
< morcos>
bc the replacement might be bad/conflicted/soemthing wrong with it
< gmaxwell>
it could, btw with bumpfee... dust limit changing between restarts.
< BlueMatt>
gmaxwell: fair...i suppose if the replacement fails to get accepted into our mempool we would want to try the original
< gmaxwell>
In that case you get to keep both pieces, but ... it's worth discussing.
< morcos>
yes this is the point... but if we skip the replacees the first time through.. then they'll get skipped by virtue of the replacer already being in the mempool on future runs, and if for some reason it isn't.. then they'll get broadcast
< morcos>
will lead to this order A'', A, A' but that's an improvement
< BlueMatt>
morcos: so you mean only do this on initial run? not on the timer-based one?
< morcos>
yes
< BlueMatt>
I'm happy with that
< morcos>
I'm compromisey with that
< BlueMatt>
it is super hacky
< morcos>
that we can agree on
< sdaftuar>
seems like a nice optimization for a futre pr
< BlueMatt>
but should at least fix the issue while not (really) risking anything
< BlueMatt>
sdaftuar: yea, which will be so chock-full of edge cases......
< BlueMatt>
but, ok
< BlueMatt>
sounds good
< morcos>
yeah can we make that a bugfix PR to 8456
< morcos>
so we can just get 8456 merged
< sdaftuar>
haha
< BlueMatt>
morcos: note how I did that to you when you asked me to change the logprints in #9375 :p
< morcos>
BlueMatt: just looking at the code.. there is actually no way for relay to happen before all the txs have tried to be accepted to the mempool right?
< cfields>
morcos: mind fixing up the mempool minReasonableRelayFee while you're messing around there? as i read the code, it doesn't respect -minrelaytxfee
< cfields>
(i'm not familiar enough with all of the interactions to know if that matters much)
< BlueMatt>
morcos: not sure? havent looked in a while?
< BlueMatt>
doesnt really fully address the issue, though
< morcos>
right still your edge case of A' no longer being able to replace A, but you think its better if you try to rebroadcast only A' in that case?
< morcos>
it seems to me if that happens, then its likely b/c you raised the required increment (probably b/c a lot of other people did too) so even if you broadcast A' it might not propagate and you might be more likely to figure that out if you have A in your mempool instead of A'
< morcos>
cfields: you are right it doesn't respect it... but it actually is better that it doesn't i think... it should probably just be cleaned up separtely in another fee estimation clean up.
< BlueMatt>
morcos: hmmmm
< cfields>
morcos: ok good, I was hoping you'd say that. So it should just take DEFAULT_MIN_RELAY_TX_FEE instead?
< morcos>
i thought through what i thought it should be a couple of weeks ago, but now i can't exactly remember....
< morcos>
almost.... i think if you ever want a minrelaytxfee less than DEFAULT, then you'd probably want that number to be less
< morcos>
but maybe we can just worry abou tthat when it happens... and probably best to just change it to DEFAULT_MIN_RELAY_TX_FEE for now
< morcos>
or its own fee estimation constant... its basically used for determining the bucket sizes... so don't want to change it b/c then your old data file is useless
< bitcoin-git>
[bitcoin] practicalswift opened pull request #9547: Avoid potential division by zero in benchmark::State::KeepRunning() (master...avoid-potential-division-by-zero-in-benchmark-state-keeprunning) https://github.com/bitcoin/bitcoin/pull/9547
< morcos>
ok maybe i should PR a change for it while we're thinking about it
< cfields>
morcos: heh, yes please :). No problem with doing it later, i just see it now and then and make a mental note to ask you, and have never managed to do so.
< BlueMatt>
morcos: I mean there's a strong argument for both - if you replaced a transaction, you really dont want to be relaying an old version out...on the flip side, if you replace a transaction and you wouldn't have accepted the replacement, maybe you want to know that...I think for my use-case (slowly bumping fee on a tx that I dont care when it confirms), I prefer the second - keep relaying the bumped one, especially because if the
< BlueMatt>
original eventually times out on other folks' mempool's, I'd want the one with ever-so-sligly-higher fee to relay out and try to get confirmed
< BlueMatt>
instead of the original one
< BlueMatt>
because I obviously bumped it for a reason
< morcos>
heh timeout is 2 weeks now.. you're a patient man
< BlueMatt>
oh, i thought we set it to 1
< BlueMatt>
oh well
< BlueMatt>
there goes that argument
< BlueMatt>
morcos: so technically the first add-all-wallet-txn-to-mempool is after CConnmanStart
< BlueMatt>
(its in postInitProcess, the very last thing called in AppInit
< BlueMatt>
)
< BlueMatt>
so you could relay out old transactions if you get connections fast and have a massive wallet
< BlueMatt>
theoretically, at least
< sdaftuar>
if we did that after the load mempool from disk finishes, that might solve a lot of the problem? m aybe messy to do that way, i haven't looked
< BlueMatt>
hmm? dont think that would make it better?
< BlueMatt>
oh, i see your point
< BlueMatt>
i mean problem is that mempool-load is so super slow...
< sdaftuar>
yeah
< sdaftuar>
but there's kind of no hurry for this is there? i was more concerned about layer violations
< morcos>
it looks to me like the first resendwallettransactions doesn't happen for 30 mins
< BlueMatt>
I mean I started by seeing the fact that your wallet might accept the pre-bump transaction into mempool after the bump as a bug....but y'all've convinced me that its at least conceivably the right outcome depending on what the user wants
< morcos>
and thats the only way they get relayed
< morcos>
just accepting them to the mempool doesn't relaythem
< BlueMatt>
i think reaccepting post-mempool-disk-load still carries risk, though - some of the wallet logic depends on mempool-reading
< BlueMatt>
so I wouldnt feel comfortable making that change without more than 2 days of review....
< gmaxwell>
BlueMatt: we already try to reaccept wallet transactions continually.
< BlueMatt>
yea?
< gmaxwell>
yea. at the rebroadcast times.
< BlueMatt>
I'm aware?
< BlueMatt>
whats your point?
< gmaxwell>
and the wallet 'works' without transactions in mempool... e.g. you can setup so that all transactions are rejected from the mempool.
< BlueMatt>
my point was that we have wallet logic which depends on whether a transaction is in mempool, and if we're gonna change it so that you could spend a bunch more time at load with transactions not yet in mempool, that would require audit and careful thought about those places
< BlueMatt>
I mean, yes, it works, but some things in it wont work
< BlueMatt>
at least last I checked
< BlueMatt>
even blocksonly puts wallet transactions in mempool, i think
< BlueMatt>
just doenst relay them
< gmaxwell>
BlueMatt: no it doesn't.
< gmaxwell>
(unless you've enabled that specifically)
< BlueMatt>
oh, heh, indeed it doesnt
< BlueMatt>
yea, ok, looking at it again it looks like it would only disable features, not enable you to do anything you shouldnt be able to
< BlueMatt>
anywayyyy
< morcos>
BlueMatt: anywayyy.......... -> ACK ?
< BlueMatt>
morcos: getting there
< BlueMatt>
morcos: ran out of steam so had to take a coffee break...digging into the meat now :p
< luke-jr>
would it be terrible, if wallets upon encounting a transaction they sent yet is still unconfirmed but is being spent already, were to double-spend their transaction to the latter destination(s)?
< luke-jr>
eg, if I pay morcos, and see morcos paying BlueMatt before mine confirms, double-spend it to BlueMatt (and morcos's change address) directly
< BlueMatt>
lol, lets not do that, I think
< BlueMatt>
that has all kinds of fun potentials
< BlueMatt>
"no, you didnt pay me, look at the blockchain!"
< luke-jr>
:D
< luke-jr>
save their transaction as proof
< BlueMatt>
of course I'd appreciate that particular scenario, because I get all the btc :p
< BlueMatt>
can we just hardcode my pubkey?
< luke-jr>
haha
< luke-jr>
would be a pain to implement in Core; maybe I'll make a highly experimental wallet that does crazy things like this if I ever get time
< sipa>
ACK hardcoding BlueMatt's pubkey
< BlueMatt>
morcos/ryanofsky: ok, so lets say you have tx A, then you bumpfee it to get A'....BUT someone has already built a tx on A (called B) (which you hadnt seen at the time of bump)....great, so now we have a scenario where, if A' times out of your mempool, it might get replaced with A (because you might see A+B from a peer prior to rebroadcasting A')
< luke-jr>
only if I get a copy of his privkey
< sdaftuar>
yes
< BlueMatt>
now you have a few goofy things - like qt's getBalance is different from wallet's GetBalance (because AvailableCoins is different from pcoin->IsTrusted())
< BlueMatt>
I know, I'm coming up with crazy edge cases now
< gmaxwell>
luke-jr: autocuttrhough could be done, I suppose but you'd want to only do it with parties that opted into it.
< ryanofsky>
don't understand the problem. if A gets added to a block then A' is conflicted and we don't care about it
< BlueMatt>
no, not in a block
< BlueMatt>
A' gets replaced in your mempool with A
< luke-jr>
so?
< BlueMatt>
(I came up with a convoluted case where that might happen, so now I'm gonna pretend we have to handle it gracefully :p)
< BlueMatt>
luke-jr: well I dont think we handle that case gracefully
< sdaftuar>
there's nothing you can really do right?
< BlueMatt>
I think in this case qt's balance will suddenly forget about both txn
< BlueMatt>
CWallet::GetBalance will just trust whatever's in your mempool, I think
< sdaftuar>
if A has a high fee child spend, then A might well be more likely to be be mined than A'
< luke-jr>
I don't see what the problem is?
< BlueMatt>
sdaftuar: sure, but your balance shouldnt be different getween rpc and gui
< gmaxwell>
sdaftuar: which change will it show in your balance?
< BlueMatt>
let alone should the gui suddenly think the balance of both options will be 0
< sdaftuar>
neither
< sdaftuar>
i think
< sdaftuar>
?
< sdaftuar>
oh, i am not sure
< BlueMatt>
but CWallet::GetBalance will show the one which is in your mempoo
< BlueMatt>
l
< gmaxwell>
E.g. A, A' then A ends up back in your mempool. Your balance doesn't go to zero when you spend coins and have change.
< gmaxwell>
(if it did users would freak often)
< BlueMatt>
rpc will list the balance assuming the one in your mempool gets confirmed, I /think/ gui would be 0
< BlueMatt>
or, at least, WalletModel::getBalance would be 0
< BlueMatt>
need to figure out what calls that
< gmaxwell>
I think it's fine if balance reads a bit low, e.g. assumes you paid the largest of the fees. It's not okay if it goes to zero.
< BlueMatt>
oh, no, its inconsistent, now I have no idea what this effects
< gmaxwell>
Because it will cause someone to freak -- "I sent 1 bitcoin and by 10 BTC balance went to zero! where are all my coins! please help. I tried deleting my wallet 5 times and they haven't come back!"
< BlueMatt>
ahh, ok, so nvm, what i think it does (because getBalance() is normally called without a coinControl) is that if you try to create a transaction it'll refuse to spend from either
< BlueMatt>
and give you an "insufficient funds" error
< BlueMatt>
but the display will be right
< gmaxwell>
yes, thats fine. it's like spendzeroconfchange=0 for those inputs.
< BlueMatt>
which is strange, but probably fine given its a crazy edge case
< BlueMatt>
gmaxwell: you asked why I hated reviewing wallet prs? :p
< BlueMatt>
billion and one possible corner cases
< gmaxwell>
if you think this is easier that reviewing network or consensus changes, I am frightened. :P
< BlueMatt>
harder than network? yea it is
< BlueMatt>
consensus, ok, maybe not
< gmaxwell>
there are just as many corner cases! we just mishandle them more often. :P
< BlueMatt>
heh, ok, fair point
< BlueMatt>
note: I still havent even fucking looked at the rpc code for bumpfee :'(
< BlueMatt>
do we document that listunspent will not list the outputs from bumped transactions (they will dissapear after you bump and neither the bumped or unbumped version's outputs will show up)
< BlueMatt>
which is super strange because it normally lists everything except abandoned outputs
< gmaxwell>
outputs disappear anytime they're used in a spend, ... bumpfee is no behavior change there.
< BlueMatt>
not in listunspent, no, I dont think so?
< gmaxwell>
any output thats spent (including in an unconfirmed txn) is not included in list_un_spent. :)
< gmaxwell>
oh do you mean the change?
< BlueMatt>
yes, change, sorry
< gmaxwell>
the change should probably still be there but marked held. :-/ basically anything in the balance computation sould be there.
< BlueMatt>
I believe its also not listed in the gui's coincontrol options for inputs
< BlueMatt>
but I'm guessing there because I'd need to go read a whole 'nother pile of code to double-check
< BlueMatt>
yes, but which one should show up in that list?
< bitcoin-git>
bitcoin/master 8017547 Matt Corallo: Make CBlockIndex*es in net_processing const
< bitcoin-git>
bitcoin/master 9a0b2f4 Matt Corallo: [qa] Make compact blocks test construction using fetch methods
< bitcoin-git>
bitcoin/master 8baaba6 Matt Corallo: [qa] Avoid race in preciousblock test....
< BlueMatt>
woooo
< bitcoin-git>
[bitcoin] sipa closed pull request #9375: Relay compact block messages prior to full block connection (master...2016-12-compact-fast-relay) https://github.com/bitcoin/bitcoin/pull/9375
< BlueMatt>
one more down for 0.14
< owowo>
how about skipping 0.14 and got directly to 0.15? I mean _4_ sounds like death in Chinese. ;0)
< morcos>
BlueMatt: ok i admit, that's some good finds. i did test some of this stuff both in RPC and GUI, but I dont think I fully thought through all the ways different balances might differ
< morcos>
Indeed change from bumper txs do not appear in coin control (as they shouldn't i think b/c you can't spend them)
< sipa>
cfields: i think we should detect whether the compiler supports -fsanitize (gcc 4.8 added -fsanitize=address, 4.9 added -fsanitize=undefined, 5.0 added -fsanitize=leak), and build test/production binaries with it to run unit tests with
< morcos>
Perhaps you are right that some of these new behaviors need to be more clearly documented
< sipa>
cfields: there is also -fsanitize=thread, which cannot be used in conjunction with any of the others
< morcos>
I'm not sure about listing the outputs in listunspent though... hmm.. Will take a look at any more comments you have tomorrow
< sipa>
cfields: downside is that both compilation and running is much slower
< bitcoin-git>
[bitcoin] kallewoof closed pull request #9235: [WIP] Refactor: Remove all uses of using namespace in all source files. (master...no-using-ns2) https://github.com/bitcoin/bitcoin/pull/9235
< BlueMatt>
morcos: I'm not sure about listing either, thats why my first request was to just document it