< BlueMatt>
hmmmmmm...i asked to use the MTP of the current block in the default importmulti timestamps because I thought that would be sufficient (+/- reorgs)...I assume #9761 is pretty much just for reorgs, then?
< gribble>
https://github.com/bitcoin/bitcoin/issues/9761 | Use 2 hour grace period for key timestamps in importmulti rescans by ryanofsky · Pull Request #9761 · bitcoin/bitcoin · GitHub
< BlueMatt>
gmaxwell: maybe?
< morcos_>
BlueMatt: the motivation of 9761 was a hypothetical situation that i didn't bother to test the reasonability of
< morcos_>
how do we think someone is going to get these birthdates
< morcos_>
in particular is it likely that you'll get it from a dumpprivkey or a validateaddress call or something else from a node where the key was created in the wallet
< morcos_>
if that wallet created timestamp is exposed, then thats not created with MTP
< morcos_>
so you need the buffer
< morcos>
i guess looking now before the recent change to validateaddress, there was no way to expose nCreateTime? but in any case there is now i think.
< morcos>
kind of a weird feature then when it was created. what were people expected to put in for the timestamp? how would they know? i guess maybe they know separately the time of the first block that was relevant?
< Arvidt_>
When running bitcoind with onlynet=onion, proxy=1.2.3.4:9050, listen=1 and externalip=xxx.onion (Tor with bitcoind hidden service xxx.onion on 1.2.3.4) can I still connect directly to bitcoind on 127.0.0.1:8333 ? How could I test that?
< sipa>
yes
< sipa>
you can test by running a second bitcoind and -connect=127.0.0.1:8333 to it
< sipa>
(with a different datadir, -port, and -rpcport)
< Arvidt_>
Thanks a lot for answer. Hm I thought more of a little telnet test, second instance is a little bit too much for me for test setup.
< sipa>
a telnet test would work too, but you'd need to construct a handshake
< sipa>
just a version message should be enough, which you could send with netcat
< bitcoin-git>
bitcoin/master a47da4b practicalswift: Use z = std::max(x - y, 0); instead of z = x - y; if (z < 0) z = 0;
< bitcoin-git>
bitcoin/master 4c69d68 Wladimir J. van der Laan: Merge #9553: Use z = std::max(x - y, 0) instead of z = x - y; if (z < 0) z = 0;...
< bitcoin-git>
[bitcoin] laanwj closed pull request #9553: Use z = std::max(x - y, 0) instead of z = x - y; if (z < 0) z = 0; (master...std-max) https://github.com/bitcoin/bitcoin/pull/9553
< bitcoin-git>
bitcoin/master a58370e Russell Yanofsky: Dedup nTimeFirstKey update logic...
< bitcoin-git>
bitcoin/master a80f98b Russell Yanofsky: Use importmulti timestamp when importing watch only keys...
< bitcoin-git>
bitcoin/master d8e8b06 Wladimir J. van der Laan: Merge #9108: Use importmulti timestamp when importing watch only keys (on top of #9682)...
< bitcoin-git>
[bitcoin] laanwj closed pull request #9108: Use importmulti timestamp when importing watch only keys (on top of #9682) (master...watchtime) https://github.com/bitcoin/bitcoin/pull/9108
< Victorsueca>
^ about that, is the function to remove watch-only addresses from your wallet coming in the near future?
< bitcoin-git>
[bitcoin] laanwj opened pull request #9764: wallet: Prevent "overrides a member function but is not marked 'override'" warnings (master...2017_02_wallet_inconsistent_missing_override) https://github.com/bitcoin/bitcoin/pull/9764
< wumpus>
Victorsueca: it needs rebase and needs tests
< BlueMatt>
morcos: ahh, so you're saying people will import with dates that dont match block time, yea, ok, that sucks
< BlueMatt>
would be nice if we had been using some mtp-or-hours-back rule to generate the birthdays to begin with :/
< BlueMatt>
oh well
< morcos>
BlueMatt: ryanofsky suggested changing the way the wallet generates the key birthday to be MTP... i couldn't immediately see why that would be a problem.., but regardless i think we need the buffer for now
< BlueMatt>
yes, well if users are going to get key birthdays from generated birthdays from old wallets then we're gonna need a buffer as long as they do that
< wumpus>
in retrospect we should have used birth block number instead of birthdate
< BlueMatt>
yes
< BlueMatt>
usability issues with that if you're not syned, but, yes
< wumpus>
the drawback is that then it's no longer possible to generate keys on hardware that has no block chain access
< wumpus>
well no big one in that case, it'd just make the birth block a bit earlier
< jonasschnelli>
but keys have timestamps as birthdays
< wumpus>
yes, it's no longer possible to do that, that's why I said in retrospect
< wumpus>
keeping a safety margin of 2 hours seems prudent to me
< BlueMatt>
indeed
< jonasschnelli>
Yes.
< jonasschnelli>
I also mentioned this in the initial importmulti PR IIRC
< bitcoin-git>
bitcoin/master 9acf25c Russell Yanofsky: Return error when importmulti called with invalid address....
< bitcoin-git>
bitcoin/master 7a93af8 Wladimir J. van der Laan: Merge #9756: Return error when importmulti called with invalid address....
< bitcoin-git>
[bitcoin] laanwj closed pull request #9756: Return error when importmulti called with invalid address. (master...pr/multiaddr) https://github.com/bitcoin/bitcoin/pull/9756
< Guest74962>
hey
< morcos>
wumpus: can we powwow on what we want to do about importmulti and pruned nodes?
< morcos>
you mentioned its the same issues as importwallet, but that is just disabled for pruned nodes, importmulti isn't
< wumpus>
importmulti should ideally work when using timestamps more recent than what is pruned
< morcos>
i think right now it'll mostly silently fail if you importmulti with a key timestamp before your earliest on disk block... (looks to me like ReadBlockFromDisk will fail and error to debug log, but rescan should keep chugging along until it finds blocks)
< morcos>
right, agreed.
< wumpus>
if that is broken then I'd say disable it for 0.14 and worry about it later
< morcos>
but important enough to fix that for 0.14?
< morcos>
ah ok, that was the question
< morcos>
so just disable importmulti for pruned nodes altogether for 0.14, then fix properly later
< morcos>
that seems reasonable to me.. gmaxwell or sipa any serious objections?
< wumpus>
my mention about the problem being the same as for importwallet was about the 2-hour grace period, we had that for importwallet and I think it should be the same for importmulti
< wumpus>
no matter how that interacts with nodes that have just pruned those two hours, it should just be documented that the grace period is there and blocks should exist for its duration until now
< wumpus>
but anyhow if importmulti doesn't even throw an error when there's not enough block data then disabling it would be better
< morcos>
right.. it's a separate edge case to be solved later if your last non-pruned block falls in the grace period.
< wumpus>
I'd assume the ReadBlockFromDisk error would not be ignored
< morcos>
just from a quick read my guess is it does not error, but i have not tried anything...
< morcos>
yeah i agree thats a problem, b/c you could think you have all the funds and not
< wumpus>
yes, silently missing blocks is a very bad
< wumpus>
important disctinction: importprivkey is only disabled in pruning mode when a rescan is requested
< wumpus>
probably should be the same for importmulti then, it should bark when any keys are not 'now', otherwise it'd lose functionality compared to importprivkey
< gmaxwell>
wumpus: it should only be disabled where the rescan would scan pruned blocks... that is really the only utility in having the timestamps at all.
< ryanofsky>
gmaxwell, are you talking about a preemptive check? would there be any problem with importing the keys then throwing an exception saying import succeeded, but some blocks were not present and some transactions might be missing?
< ryanofsky>
also i don't understand what's wrong with the suggestion from wumpus to just forbid rescan=True on pruned nodes
< gmaxwell>
Because it guts the utility of pruning, when you lose major functionality outright just because you were so foolish as to not waste 100GB of diskspace.
< gmaxwell>
Esp because rescans of the whole chain take hours, so it's not like they were even all that usable (except in emergencies) in any case.
< gmaxwell>
As far as after the fact, I think that would be way less bad than denying it completely.
< gmaxwell>
it would be best to deny it first but the way importmulti is constructed makes that harder to implement.
< ryanofsky>
oh ok, i got that part, i didn't understand what you were saying about the defeating the "utility of timestamps"
< gmaxwell>
Bascially the only good timestamps do is that they let you import a key without reading all the blocks. (scanning extra blocks is something no one would care about.) If pruning is prevented then you do save some rescan time, but still take on the 100GB of cost, -- this is a major barrier for people doing their own payment processing on VPSes.
< gmaxwell>
The workflow there is that they have a payment processing front end which generates keys using BIP32 public derrivation, and then imports the addresses. Because of contingencies around multiple nodes and restarts they may need to rescan a bit. The imported keys are used to watch for payments so they can display status to the user.
< gmaxwell>
The actual private keys are not put onto the VPS systems, for obvious reasons. :)
< gmaxwell>
The alternative to this setup is to use a third party payment processor. But this has the downside of introducing trust where it could be avoided, and all the major processors are known to capricious shut down merchants or block customers on the basis of flimsy risk (that they piss off banks or regulators) analysis.
< gmaxwell>
(and, of course, the systemic risk created by those processors holding large amounts of customer funds).
< cfields>
gmaxwell: I'm going to add a little blurb about the net speedup in the changelog. I think i remember you mentioning you'd prefer to see some sort of "validation improvements" or "ibd speedups" section, which may be a more useful way to describe it?
< morcos>
Should we be making it so POTENTIAL DEADLOCK DETECTED's are elimintated or is it impossible to eliminate them all
< morcos>
I was just trackign down what caused one and I think its not an issue, but is there somehwere I should comment what caused it, or should we actually change the code to not lock incorrectly
< morcos>
i'm just not very familar with DEBUG_LOCKORDER
< sipa>
i think we can replace it with tsan soon
< sipa>
and/or helgrind
< morcos>
This particular case is the loading of a wallet can call Mark Conflicted, which causes cs_wallet then cs_main which is the opposite order it happens elsewhere
< sipa>
what is the other case?
< morcos>
we have tons of LOCK2(cs_main, cs_wallet)'s
< morcos>
i was assuming this isn't a problem b/c the wallet loading is done before other threads are spun up, but maybe thats actually not right?
< sipa>
well the only question is whether those 2 orders can occur simultaneously
< morcos>
yes thats what i mean, can anything else happen while the wallet is still loading?
< morcos>
dpesm
< morcos>
oops
< morcos>
doesn't look like it i suppose...
< sipa>
regardless, i think we should have consistent lock orders everywhere
< sipa>
i'd say all those LOCK2(cs_main, cs_wallet)s can be swapped
< morcos>
yikes!!
< sipa>
wallet should call main, not the other way around IMHO
< morcos>
i bet several of them are reentrant
< morcos>
main is already held before we get there
< sipa>
well, that's the issue then
< sipa>
main shouldn't be held while calling wallet code
< morcos>
it assert fails when you have a potential deadlock?
< morcos>
so this must have been somehow recently introduced?
< cfields>
mm, I'm going to go ahead and PR some mutex cleanups I've been waiting on for a while. I'm going to start moving the net code towards non-recursive locks. Would you guys prefer a generic LOCK(cs) that takes any mutex, or NON_RECURSIVE_LOCK(cs) as a form of self-documentation?
< morcos>
so... what should i do about this stupid deadlock warnign for now? seems like it might annoy someone else.. but only obvious fix i could see would be holding cs_main for all of loadwallet. its not immediately apparent to me whether that is bad?
< morcos>
eh.. i'll move on and make an issue.. if you have a clean wallet, i think the problem goes away
< bitcoin-git>
[bitcoin] ryanofsky opened pull request #9771: Add missing cs_wallet lock that triggers new lock held assertion (master...pr/loadlock) https://github.com/bitcoin/bitcoin/pull/9771
< bitcoin-git>
[bitcoin] ryanofsky opened pull request #9773: WIP: Return errors from importmulti if complete rescans are not successful (master...pr/multicheck) https://github.com/bitcoin/bitcoin/pull/9773