< bitcoin-git> [bitcoin] jonasschnelli pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/b4a509a3f817...093074b84395
< 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
< jonasschnelli> ping cfields
< bitcoin-git> [bitcoin] MarcoFalke pushed 3 new commits to master: https://github.com/bitcoin/bitcoin/compare/093074b84395...9ccafb1d7bdd
< bitcoin-git> bitcoin/master fd86f99 MarcoFalke: Squashed 'src/secp256k1/' changes from 84973d393..0b7024185...
< bitcoin-git> bitcoin/master 999968e MarcoFalke: Bump secp256k1 subtree
< bitcoin-git> bitcoin/master 9ccafb1 MarcoFalke: Merge #11421: Merge current secp256k1 subtree...
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #11421: Merge current secp256k1 subtree (master...Mf1709-subtree-secp256k1) https://github.com/bitcoin/bitcoin/pull/11421
< bitcoin-git> [bitcoin] laanwj pushed 3 new commits to master: https://github.com/bitcoin/bitcoin/compare/9ccafb1d7bdd...a4c833fec104
< bitcoin-git> bitcoin/master fae2673 MarcoFalke: qa: check-rpc-mapping must not run on empty lists
< bitcoin-git> bitcoin/master fae60e3 MarcoFalke: qa: Fix lcov for out-of-tree builds
< bitcoin-git> bitcoin/master a4c833f Wladimir J. van der Laan: Merge #11443: [qa] Allow "make cov" out-of-tree; Fix rpc mapping check...
< bitcoin-git> [bitcoin] laanwj closed pull request #11443: [qa] Allow "make cov" out-of-tree; Fix rpc mapping check (master...Mf1710-qaFixups) https://github.com/bitcoin/bitcoin/pull/11443
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/a4c833fec104...e12522dfdaab
< bitcoin-git> bitcoin/master 6643b80 Matt Corallo: Add state message print to AcceptBlock failure message....
< bitcoin-git> bitcoin/master e12522d Wladimir J. van der Laan: Merge #11406: Add state message print to AcceptBlock failure message....
< bitcoin-git> [bitcoin] laanwj closed pull request #11406: Add state message print to AcceptBlock failure message. (master...2017-09-checkblock-fail-print) https://github.com/bitcoin/bitcoin/pull/11406
< bitcoin-git> [bitcoin] laanwj pushed 3 new commits to master: https://github.com/bitcoin/bitcoin/compare/e12522dfdaab...a1f7f1870931
< bitcoin-git> bitcoin/master 6fb8f5f practicalswift: Check that -blocknotify command is non-empty before executing...
< bitcoin-git> bitcoin/master cffe85f practicalswift: Skip sys::system(...) call in case of empty command
< bitcoin-git> bitcoin/master a1f7f18 Wladimir J. van der Laan: Merge #10939: [init] Check non-emptiness of -blocknotify command prior to executing...
< bitcoin-git> [bitcoin] laanwj closed pull request #10939: [init] Check non-emptiness of -blocknotify command prior to executing (master...blocknotify-consistentcy) https://github.com/bitcoin/bitcoin/pull/10939
< bitcoin-git> [bitcoin] laanwj closed pull request #10233: Wallet: Support not reusing addresses (master...freezea) https://github.com/bitcoin/bitcoin/pull/10233
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/a1f7f1870931...7f11ef260855
< bitcoin-git> bitcoin/master 0cd9273 Wladimir J. van der Laan: rpc: Prevent `dumpwallet` from overwriting files...
< bitcoin-git> bitcoin/master 7f11ef2 Wladimir J. van der Laan: Merge #9937: rpc: Prevent `dumpwallet` from overwriting files...
< bitcoin-git> [bitcoin] laanwj closed pull request #9937: rpc: Prevent `dumpwallet` from overwriting files (master...2017_03_walletdump_nooverwrite) https://github.com/bitcoin/bitcoin/pull/9937
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/7f11ef260855...74123eabdd91
< bitcoin-git> bitcoin/master 96c2ce9 Matt Corallo: Fix validationinterface build on super old boost/clang...
< bitcoin-git> bitcoin/master 74123ea Wladimir J. van der Laan: Merge #11440: Fix validationinterface build on super old boost/clang...
< bitcoin-git> [bitcoin] laanwj closed pull request #11440: Fix validationinterface build on super old boost/clang (master...2017-10-cblock-validationinterface) https://github.com/bitcoin/bitcoin/pull/11440
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/74123eabdd91...167cef8082e2
< 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
< bitcoin-git> [bitcoin] laanwj closed pull request #11046: Replace boost sleep_for with C++11 equivalent (master...2017/08/boost_sleep_for) https://github.com/bitcoin/bitcoin/pull/11046
< bitcoin-git> [bitcoin] laanwj closed pull request #10990: 0 locktime issue (master...fix0locktimebug) https://github.com/bitcoin/bitcoin/pull/10990
< promag> should we have only one BitcoinTestFramework subclass in each test file?
< MarcoFalke> I'd prefer so. Otherwise you'd have to run them sequentially and a failure of the first causes the others not to run.
< MarcoFalke> Also would be confusing to have (potentially) different topologies set in a single file
< promag> Regarding #10941, I think we can merge -(alert|block|wallet)notify tests later
< gribble> https://github.com/bitcoin/bitcoin/issues/10941 | Add blocknotify functional test by promag · Pull Request #10941 · bitcoin/bitcoin · GitHub
< 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)... :)
< gribble> https://github.com/bitcoin/bitcoin/issues/7061 | [Wallet] Add RPC call "rescanblockchain " by jonasschnelli · Pull Request #7061 · bitcoin/bitcoin · GitHub
< jonasschnelli> Needs some reacks
< meshcollider> How long does it normally take before changes to bitcoincore.org repo go live
< cfields> jonasschnelli: very late pong
< jonasschnelli> Heh...
< jonasschnelli> It's now in a comment... let me link you
< jonasschnelli> meshcollider: ask BlueMatt. But the recent merged 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 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> Okay. Updated the PR
< jonasschnelli> ping JeremyRu1in
< 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)
< JeremyRubin> jonasschnelli: I think that seems correct to me. If you have no failures reported, then all the blocks were scanned in that range
< jonasschnelli> Yes. If you get this (new) error, you may have scanned some of the blocks (but we won't tell which one)
< JeremyRubin> Correct.
< jonasschnelli> If you have a corrupted block you are alreafy in serious troubles... :)
< JeremyRubin> Yes. Maybe worth saying 'Rescan Failed. Potentially corruputed data files.'
< 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> so remove the return values
< jonasschnelli> the throw will lead to not return the range
< JeremyRubin> Actually... making abortrescan return the range seems like good UX
< jonasschnelli> We could add that later,.. seems out of scope and related to #11450
< gribble> https://github.com/bitcoin/bitcoin/issues/11450 | ScanForWalletTransactions return value is incorrectly documented · Issue #11450 · bitcoin/bitcoin · GitHub
< JeremyRubin> k -- so otherwise, you should get rid of the return values.
< jtimon> I really don't get it, if someone else can try to explain, that would be welcomed re https://github.com/bitcoin/bitcoin/pull/11427#issuecomment-334033122
< 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
< bitcoin-git> [bitcoin] promag opened pull request #11452: Improve ZMQ functional test (master...2017-10-improve-zmq-test) https://github.com/bitcoin/bitcoin/pull/11452