< bitcoin-git>
bitcoin/master ab5bba7 Alejandro Avilés: Fix launchctl not being able to stop bitcoind...
< bitcoin-git>
bitcoin/master 093074b Jonas Schnelli: Merge #11419: Utils: Fix launchctl not being able to stop bitcoind...
< bitcoin-git>
[bitcoin] jonasschnelli closed pull request #11419: Utils: Fix launchctl not being able to stop bitcoind (master...fix-macos-init) https://github.com/bitcoin/bitcoin/pull/11419
< bitcoin-git>
bitcoin/master f35d033 practicalswift: build: Make "make clean" remove all files created when running "make check"...
< bitcoin-git>
bitcoin/master 167cef8 Wladimir J. van der Laan: Merge #11435: build: Make "make clean" remove all files created when running "make check"...
< bitcoin-git>
[bitcoin] laanwj closed pull request #11435: build: Make "make clean" remove all files created when running "make check" (master...make-cleaner) https://github.com/bitcoin/bitcoin/pull/11435
< promag>
but at the moment each one has different network setups and params
< promag>
I think merging those tests deserve a separate PR
< bitcoin-git>
[bitcoin] promag closed pull request #11439: [test] Refactor ZMQ test to use one address per notification type (master...2017-10-clean-zmq-test) https://github.com/bitcoin/bitcoin/pull/11439
< promag>
jnewbery: ScanForWalletTransactions interface is a bit weird
< promag>
I think it should return the last block scanned
< promag>
and also *all* failed blocks
< promag>
not sure why only the most recent is useful
< jonasschnelli>
Hopefully we can merge #7061 soon. (178 comments, open since almost two years)... :)
< jonasschnelli>
meshcollider: ask BlueMatt. But the recent merged 0.15.0.1 changes are online
< BlueMatt>
meshcollider: like....60 seconds?
< BlueMatt>
unless someone fucked up the commit signatures, which folks are want to do
< BlueMatt>
err, wont to do
< cfields>
jonasschnelli: sounds good to me
< jonasschnelli>
okay... there is still an issue... will fix soon, so wait with looking at the code
< cfields>
I'd really rather see fLimitedNode in CNodeState though :(
< cfields>
BlueMatt: thoughts on starting to migrate that way?
< cfields>
(see above link for reference)
< BlueMatt>
whats context?
< cfields>
BlueMatt: a bool to indicate whether or not a node is limited. Stored during version. Pretty much same use case as fPreferred
< meshcollider>
BlueMatt: jonasschnelli Oh I didn't see it because the 0.15.0.1 release announcement isn't in the recent posts list
< cfields>
I think just used to avoid fetching old blocks. presumably jonasschnelli stuck it in CNode because that's where fClient lives (but shouldn't)
< BlueMatt>
meshcollider: we only fixed it like an hour or three ago
< jonasschnelli>
cfields: would always checking nServices be terrible? (instead of caching a boolen)?
< BlueMatt>
cfields: so, wait, you want to add an fLimitedNode in addition to the fPreferred stuff? why not just adapt UpdatePreferredDownload to consider limited-ness and call it on our peers in the first UpdatedBlockTip with not-fInitialDownload?
< BlueMatt>
you may be stuck in ibd for a while, there isnt much point in filling your outbound peers with limited nodes for that duration
< BlueMatt>
or am I missing something?
< BlueMatt>
grr, I'll go review the pr and give real feedback instead of bullshitting, give me 20 minutes
< cfields>
My initial thought was to tie it to preferred as well, but iirc that broke down somewhere
< cfields>
I was also thinking that we'd want to avoid pruned nodes for initial headers sync, but I guess that's not really necessary
< cfields>
BlueMatt: generally though, I really dislike expanding the scope of variables like that. eventualy fPreferred turns into something entirely meaningless like "whitelisted"
< jonasschnelli>
My re-check / re-test told me, rescanblockchain { start_height: 10, stop_height: 20 } does also rescan block 20
< JeremyRubin>
jonasschnelli: it doesn't work; see PR comment
< JeremyRubin>
it's the failure mode that's broken, I should have been more clear
< JeremyRubin>
but I was confused about the general range semantics too; so more clear language is good there nonetheless
< JeremyRubin>
jonasschnelli: try scanning from 10 - 20 on a healthy node
< JeremyRubin>
then, corrupt block 20 so it won't scan.
< JeremyRubin>
now, rescan again
< JeremyRubin>
same result
< JeremyRubin>
now, corrupt blocks 10-20
< JeremyRubin>
will return the same thing
< jonasschnelli>
JeremyRubin: I added a LogPrintf below the ReadBlockFromDisk and AddToWalletIfInvolvingMe scan... and block 20 was scanned.
< jonasschnelli>
But maybe I should try your setup
< BlueMatt>
jonasschnelli: will you kill me if I suggest a cleanup to nRequiredServices/nRelevantServices as a first commit in your pr?
< * BlueMatt>
feels like he's pulling a cfields here :p
< JeremyRubin>
jonasschnelli: Yes, if your scanning does not fail, it is fine.
< jonasschnelli>
BlueMatt: ideally we keep cleanups in a seperate PR
< BlueMatt>
not like cfields ever requires it for others, but endless first-commit cleanups
< JeremyRubin>
But if you have a corruption, it is not.
< jonasschnelli>
BlueMatt: but I can cherry pick.. :)
< cfields>
heh
< BlueMatt>
jonasschnelli: let me play around a bit
< jonasschnelli>
JeremyRubin: what do you mean with corruption?
< cfields>
BlueMatt: only allowed if you first ack my socket changes you required :p
< jonasschnelli>
invalidateblock?
< ryanofsky>
jonasschnelli, if readblockfrom disk fails
< jonasschnelli>
BlueMatt: yeah. Happy to pull-in a cleanup into bip159 PR
< BlueMatt>
cfields: damn you
< cfields>
jonasschnelli: oh that reminds me, what do you think about adding "NODE_NETWORK should be combined with NODE_NETWORK_LIMITED" to the bip (if not already) ?
< cfields>
I don't think that's just an implementation detail
< jonasschnelli>
JeremyRubin, ryanofsky: aha... a filed ReadBlockFromDisk falsely reports a successful scan.
< jonasschnelli>
cfields: Yeah. Need to add this... is unclear in the BIP
< JeremyRubin>
No
< jonasschnelli>
*a failed
< JeremyRubin>
It correctly reports an unsuccessful scan
< jonasschnelli>
yeah.. what I meant. sry
< JeremyRubin>
you incorrectly interpret that result :)
< JeremyRubin>
basically ANY non-nullptr return should cause you to return "scan failed"
< jonasschnelli>
JeremyRubin: this would also be undetected in classic startup -rescans? Right?
< JeremyRubin>
Unsure of prior semantics of rescan
< JeremyRubin>
it would be reported that you have only succeeded in scanning blocks after LAST_FAILED, so then you would need to rescan up to LAST_FAILED
< jonasschnelli>
the current rescan function skips corrupted blocks IMO
< JeremyRubin>
Yes, but it also tells you 'no guarantees before block N'
< JeremyRubin>
where N = LAST_FAILED
< JeremyRubin>
this impl, however, is happy to tell you Blocks A-B succeded
< JeremyRubin>
which is incorrect
< jonasschnelli>
I see.. let me see how we can fix this.
< JeremyRubin>
you have 2 choices I think
< jonasschnelli>
I don't want to change the way how current startup -rescans work..
< JeremyRubin>
either keep a vector of all failures
< jonasschnelli>
aborting on a corrupted block seems not to be the ideal choice
< JeremyRubin>
if passed in as a ptr arg with default nullptr
< JeremyRubin>
And return a vector of failures
< jonasschnelli>
or report "could not read all block" (bool)?
< JeremyRubin>
That could work too...
< JeremyRubin>
I would (personally) not return *anything* if there are any failures.
< jonasschnelli>
AFAIK, ScanForWalletTransactions returns non-nullptr when detecting a corrupt block.. could use that
< JeremyRubin>
yes, that's fine. I would return a `Maybe (Int,Int)`, and only return `Some (start, stop)` if there are no failures.
< JeremyRubin>
I would also push you to rework the errors to differentiate when the numbers are actually invalid v.s. out of range for the current chain we're on.
< JeremyRubin>
Because asking for (1000, 2000) could fail depending on how caught up you are.
< JeremyRubin>
But is not an invalid request like (2000,1000), (-10, 20), etc.
< JeremyRubin>
(in particular, because I think that it would be useful to issue a rescan from, let's say, (tip-10, tip+1000) and for it to return (tip-10, tip)
< jonasschnelli>
JeremyRubin: Thanks... let me add this
< JeremyRubin>
jonasschnelli: my comment about handling of invalid ranges though still stands. differentiating between the cases of overlap is good, an you can return useful results in some of them.
< jonasschnelli>
JeremyRubin: can you rephrase? I don't get your range concern
< JeremyRubin>
bad ranges: ()[] ([)] good ranges: [()] ok ranges: [(]) []()
< jonasschnelli>
?
< JeremyRubin>
(let '(' and ')' denote the start and stop range, and '[' and ']' denote genesis to tip on a number line)
< jonasschnelli>
aha...
< JeremyRubin>
so if i ask for (tip -10, tip+100) you should return scanned (tip-10, tip)
< jonasschnelli>
stop_height is optional but start_heigt is required when stop_height is passed
< jonasschnelli>
I think tip+100 should result in an error
< JeremyRubin>
I don't
< JeremyRubin>
because it could result from our node just being behind and syncing
< jonasschnelli>
Yeah.. but would you silently ignore it? IMO it deserves that we inform the user (with an error)
< JeremyRubin>
If you want it to be an error, it shouldn't be invalid it should be 'not synced to X yet'
< jonasschnelli>
Then he can self-correct rather then auto-correct
< JeremyRubin>
if you want it to be more clear, then you should always return the current tip
< JeremyRubin>
but in the 'ok ranges' cases, the type of failure is very different from the bad ranges
< JeremyRubin>
bad ranges will NEVER succeed, ok ranges will succeed if you wait long enough
< JeremyRubin>
So bad ranges are invalid. ok ranges are too 'soon' for our node.
< JeremyRubin>
And if you don't want to support ok ranges, then there is no point of returning the range scanned at all, you may as well return `Maybe ()` because success means the range was scanned.
< JeremyRubin>
((unless I'm not seeing some edge case? Maybe if you somehow trigger an abortrescan you would return (start, point_aborted)?))
< jonasschnelli>
JeremyRubin: but with the current fix (throw when getting a non-nullptr) there is a guarantee we have scanned the given range, right?
< JeremyRubin>
Correct!
< jonasschnelli>
Do you see anything else t
< jonasschnelli>
to fix?
< JeremyRubin>
((Except see above abortrescan concern))
< jonasschnelli>
abortrescan leads to an explicit error thrown
< JeremyRubin>
ok -- so then no point in returning the range
< JeremyRubin>
Unless you want to handle the ranges I've labelled 'ok'
< jonasschnelli>
You are aware that a *throw" will lead to return nothing blow that acctuall throw code line?
< JeremyRubin>
(which, even if not handled, I think merit a separate designation from 'invalid')
< JeremyRubin>
jonasschnelli: I'm merely pointing out that the absence of an error indicates that the request completed as requested, so the caller already knows start and stop.
< JeremyRubin>
But maybe it's better interface design to not rely on that in case we improve the behavior in the future ;)
< jonasschnelli>
JeremyRubin: hm... sorry for not following completely... which absence of an error you referring to?
< JeremyRubin>
Ok. So you're a client. You ask for rescan (tip-10,tip-5).
< JeremyRubin>
I'm the server.
< JeremyRubin>
I see the request, and I rescan happily.
< JeremyRubin>
I now will return a range (start, stop) based on your request.
< JeremyRubin>
Under what circumstance do you, the caller, not already know what that range is?
< JeremyRubin>
I suppose you don't know the tip if you only pass a start?
< JeremyRubin>
So maybe it merits returning just the finish height, but you never don't know what start you requested already.
< JeremyRubin>
Anyways... this is secondary, it's not really an issue. But I think that handling ok ranges addresses my concern anyways.
< jonasschnelli>
JeremyRubin: Okay. I see you point.
< jonasschnelli>
I guess we could leave it away completely then.... or keep it for future extenions...
< jonasschnelli>
Although, if you keep away the stop height,... you should at leat get the height of the scanned tip
< jonasschnelli>
Which may be important
< JeremyRubin>
I think that handling ok ranges in the manner suggested gets this to have the correct semantics for the returned data
< jonasschnelli>
JeremyRubin: thanks for reviewing and helping me to understand your point. :)
< JeremyRubin>
yeah, sorry if my explanations were unclear
< JeremyRubin>
It was a pretty subtle bug I guess, because a lot of people missed it!
< jonasschnelli>
Re-reading your comments looks like I had my eyes covered with tomatos
< jonasschnelli>
Yes. Glad you did a review.. and tell me, if you think other stuff needs to be changed/fixed.
< JeremyRubin>
jonasschnelli: I would like to see ok_ranges handled in the code and then I'd ack it. If you feel strongly that they should not attempt to scan, and just fail, that's fine. But it should at least return different error strings (e.g., 'start is greater than tip', 'end is greater than tip', 'start must be positive', 'end must be positive')
< JeremyRubin>
but I think that it is simple to say that at least the end being greater than the tip has an obvious execution (execute as if no argument were provided).
< JeremyRubin>
Start being greater than the tip requires some notion of an empty scan, which I don't think you can express easily with an inclusive range (unless you do another optional bool not_synced_fully).
< JeremyRubin>
jonasschnelli: ^^^ afk soon, shoot an email if questions