1 2017-10-04 03:15:38	0|bitcoin-git|13bitcoin/06master 14ab5bba7 15Alejandro Avilés: Fix launchctl not being able to stop bitcoind...
  2 2017-10-04 03:15:38	0|bitcoin-git|[13bitcoin] 15jonasschnelli pushed 2 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/b4a509a3f817...093074b84395
  3 2017-10-04 03:15:39	0|bitcoin-git|13bitcoin/06master 14093074b 15Jonas Schnelli: Merge #11419: Utils: Fix launchctl not being able to stop bitcoind...
  4 2017-10-04 03:16:19	0|bitcoin-git|[13bitcoin] 15jonasschnelli closed pull request #11419: Utils: Fix launchctl not being able to stop bitcoind (06master...06fix-macos-init) 02https://github.com/bitcoin/bitcoin/pull/11419
  5 2017-10-04 04:13:04	0|jonasschnelli|ping cfields
  6 2017-10-04 10:40:05	0|bitcoin-git|[13bitcoin] 15MarcoFalke pushed 3 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/093074b84395...9ccafb1d7bdd
  7 2017-10-04 10:40:06	0|bitcoin-git|13bitcoin/06master 14999968e 15MarcoFalke: Bump secp256k1 subtree
  8 2017-10-04 10:40:06	0|bitcoin-git|13bitcoin/06master 14fd86f99 15MarcoFalke: Squashed 'src/secp256k1/' changes from 84973d393..0b7024185...
  9 2017-10-04 10:40:07	0|bitcoin-git|13bitcoin/06master 149ccafb1 15MarcoFalke: Merge #11421: Merge current secp256k1 subtree...
 10 2017-10-04 10:40:40	0|bitcoin-git|[13bitcoin] 15MarcoFalke closed pull request #11421: Merge current secp256k1 subtree (06master...06Mf1709-subtree-secp256k1) 02https://github.com/bitcoin/bitcoin/pull/11421
 11 2017-10-04 12:26:29	0|bitcoin-git|[13bitcoin] 15laanwj pushed 3 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/9ccafb1d7bdd...a4c833fec104
 12 2017-10-04 12:26:30	0|bitcoin-git|13bitcoin/06master 14fae2673 15MarcoFalke: qa: check-rpc-mapping must not run on empty lists
 13 2017-10-04 12:26:30	0|bitcoin-git|13bitcoin/06master 14fae60e3 15MarcoFalke: qa: Fix lcov for out-of-tree builds
 14 2017-10-04 12:26:31	0|bitcoin-git|13bitcoin/06master 14a4c833f 15Wladimir J. van der Laan: Merge #11443: [qa] Allow "make cov" out-of-tree; Fix rpc mapping check...
 15 2017-10-04 12:27:12	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #11443: [qa] Allow "make cov" out-of-tree; Fix rpc mapping check (06master...06Mf1710-qaFixups) 02https://github.com/bitcoin/bitcoin/pull/11443
 16 2017-10-04 12:36:38	0|bitcoin-git|13bitcoin/06master 146643b80 15Matt Corallo: Add state message print to AcceptBlock failure message....
 17 2017-10-04 12:36:38	0|bitcoin-git|[13bitcoin] 15laanwj pushed 2 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/a4c833fec104...e12522dfdaab
 18 2017-10-04 12:36:39	0|bitcoin-git|13bitcoin/06master 14e12522d 15Wladimir J. van der Laan: Merge #11406: Add state message print to AcceptBlock failure message....
 19 2017-10-04 12:37:20	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #11406: Add state message print to AcceptBlock failure message. (06master...062017-09-checkblock-fail-print) 02https://github.com/bitcoin/bitcoin/pull/11406
 20 2017-10-04 12:54:42	0|bitcoin-git|[13bitcoin] 15laanwj pushed 3 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/e12522dfdaab...a1f7f1870931
 21 2017-10-04 12:54:43	0|bitcoin-git|13bitcoin/06master 146fb8f5f 15practicalswift: Check that -blocknotify command is non-empty before executing...
 22 2017-10-04 12:54:43	0|bitcoin-git|13bitcoin/06master 14cffe85f 15practicalswift: Skip sys::system(...) call in case of empty command
 23 2017-10-04 12:54:44	0|bitcoin-git|13bitcoin/06master 14a1f7f18 15Wladimir J. van der Laan: Merge #10939: [init] Check non-emptiness of -blocknotify command prior to executing...
 24 2017-10-04 12:55:11	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #10939: [init] Check non-emptiness of -blocknotify command prior to executing (06master...06blocknotify-consistentcy) 02https://github.com/bitcoin/bitcoin/pull/10939
 25 2017-10-04 12:59:10	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #10233: Wallet: Support not reusing addresses (06master...06freezea) 02https://github.com/bitcoin/bitcoin/pull/10233
 26 2017-10-04 13:01:52	0|bitcoin-git|13bitcoin/06master 140cd9273 15Wladimir J. van der Laan: rpc: Prevent `dumpwallet` from overwriting files...
 27 2017-10-04 13:01:52	0|bitcoin-git|[13bitcoin] 15laanwj pushed 2 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/a1f7f1870931...7f11ef260855
 28 2017-10-04 13:01:53	0|bitcoin-git|13bitcoin/06master 147f11ef2 15Wladimir J. van der Laan: Merge #9937: rpc: Prevent `dumpwallet` from overwriting files...
 29 2017-10-04 13:02:02	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #9937: rpc: Prevent `dumpwallet` from overwriting files (06master...062017_03_walletdump_nooverwrite) 02https://github.com/bitcoin/bitcoin/pull/9937
 30 2017-10-04 13:03:07	0|bitcoin-git|13bitcoin/06master 1496c2ce9 15Matt Corallo: Fix validationinterface build on super old boost/clang...
 31 2017-10-04 13:03:07	0|bitcoin-git|[13bitcoin] 15laanwj pushed 2 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/7f11ef260855...74123eabdd91
 32 2017-10-04 13:03:08	0|bitcoin-git|13bitcoin/06master 1474123ea 15Wladimir J. van der Laan: Merge #11440: Fix validationinterface build on super old boost/clang...
 33 2017-10-04 13:03:53	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #11440: Fix validationinterface build on super old boost/clang (06master...062017-10-cblock-validationinterface) 02https://github.com/bitcoin/bitcoin/pull/11440
 34 2017-10-04 13:36:10	0|bitcoin-git|13bitcoin/06master 14f35d033 15practicalswift: build: Make "make clean" remove all files created when running "make check"...
 35 2017-10-04 13:36:10	0|bitcoin-git|[13bitcoin] 15laanwj pushed 2 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/74123eabdd91...167cef8082e2
 36 2017-10-04 13:36:11	0|bitcoin-git|13bitcoin/06master 14167cef8 15Wladimir J. van der Laan: Merge #11435: build: Make "make clean" remove all files created when running "make check"...
 37 2017-10-04 13:36:50	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #11435: build: Make "make clean" remove all files created when running "make check" (06master...06make-cleaner) 02https://github.com/bitcoin/bitcoin/pull/11435
 38 2017-10-04 13:38:15	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #11046: Replace boost sleep_for with C++11 equivalent (06master...062017/08/boost_sleep_for) 02https://github.com/bitcoin/bitcoin/pull/11046
 39 2017-10-04 13:39:35	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #10990: 0 locktime issue (06master...06fix0locktimebug) 02https://github.com/bitcoin/bitcoin/pull/10990
 40 2017-10-04 13:57:58	0|promag|should we have only one BitcoinTestFramework subclass in each test file?
 41 2017-10-04 14:02:25	0|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.
 42 2017-10-04 14:03:03	0|MarcoFalke|Also would be confusing to have (potentially) different topologies set in a single file
 43 2017-10-04 14:03:52	0|promag|Regarding #10941, I think we can merge -(alert|block|wallet)notify tests later
 44 2017-10-04 14:03:54	0|gribble|https://github.com/bitcoin/bitcoin/issues/10941 | Add blocknotify functional test by promag · Pull Request #10941 · bitcoin/bitcoin · GitHub
 45 2017-10-04 14:04:08	0|promag|but at the moment each one has different network setups and params
 46 2017-10-04 14:05:51	0|promag|I think merging those tests deserve a separate PR
 47 2017-10-04 14:50:54	0|bitcoin-git|[13bitcoin] 15promag closed pull request #11439: [test] Refactor ZMQ test to use one address per notification type (06master...062017-10-clean-zmq-test) 02https://github.com/bitcoin/bitcoin/pull/11439
 48 2017-10-04 14:54:23	0|promag|jnewbery: ScanForWalletTransactions interface is a bit weird
 49 2017-10-04 14:55:00	0|promag|I think it should return the last block scanned
 50 2017-10-04 14:55:16	0|promag|and also *all* failed blocks
 51 2017-10-04 14:55:29	0|promag|not sure why only the most recent is useful
 52 2017-10-04 17:56:25	0|jonasschnelli|Hopefully we can merge #7061 soon. (178 comments, open since almost two years)... :)
 53 2017-10-04 17:56:29	0|gribble|https://github.com/bitcoin/bitcoin/issues/7061 | [Wallet] Add RPC call "rescanblockchain " by jonasschnelli · Pull Request #7061 · bitcoin/bitcoin · GitHub
 54 2017-10-04 17:56:30	0|jonasschnelli|Needs some reacks
 55 2017-10-04 19:23:43	0|meshcollider|How long does it normally take before changes to bitcoincore.org repo go live
 56 2017-10-04 19:37:22	0|cfields|jonasschnelli: very late pong
 57 2017-10-04 19:37:42	0|jonasschnelli|Heh...
 58 2017-10-04 19:37:57	0|jonasschnelli|It's now in a comment... let me link you
 59 2017-10-04 19:38:11	0|jonasschnelli|https://github.com/bitcoin/bitcoin/pull/10387#pullrequestreview-66842269
 60 2017-10-04 19:38:42	0|jonasschnelli|meshcollider: ask BlueMatt. But the recent merged 0.15.0.1 changes are online
 61 2017-10-04 19:39:13	0|BlueMatt|meshcollider: like....60 seconds?
 62 2017-10-04 19:39:37	0|BlueMatt|unless someone fucked up the commit signatures, which folks are want to do
 63 2017-10-04 19:39:54	0|BlueMatt|err, wont to do
 64 2017-10-04 19:43:45	0|cfields|jonasschnelli: sounds good to me
 65 2017-10-04 19:44:08	0|jonasschnelli|okay... there is still an issue... will fix soon, so wait with looking at the code
 66 2017-10-04 19:44:26	0|cfields|I'd really rather see fLimitedNode in CNodeState though :(
 67 2017-10-04 19:44:39	0|cfields|BlueMatt: thoughts on starting to migrate that way?
 68 2017-10-04 19:44:59	0|cfields|(see above link for reference)
 69 2017-10-04 19:45:05	0|BlueMatt|whats context?
 70 2017-10-04 19:46:13	0|cfields|BlueMatt: a bool to indicate whether or not a node is limited. Stored during version. Pretty much same use case as fPreferred
 71 2017-10-04 19:48:39	0|meshcollider|BlueMatt: jonasschnelli Oh I didn't see it because the 0.15.0.1 release announcement isn't in the recent posts list
 72 2017-10-04 19:48:44	0|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)
 73 2017-10-04 19:49:09	0|BlueMatt|meshcollider: we only fixed it like an hour or three ago
 74 2017-10-04 19:49:33	0|jonasschnelli|cfields: would always checking nServices be terrible? (instead of caching a boolen)?
 75 2017-10-04 19:49:51	0|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?
 76 2017-10-04 19:52:47	0|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
 77 2017-10-04 19:52:51	0|BlueMatt|or am I missing something?
 78 2017-10-04 19:54:51	0|BlueMatt|grr, I'll go review the pr and give real feedback instead of bullshitting, give me 20 minutes
 79 2017-10-04 19:55:01	0|cfields|My initial thought was to tie it to preferred as well, but iirc that broke down somewhere
 80 2017-10-04 19:56:55	0|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
 81 2017-10-04 19:58:36	0|cfields|BlueMatt: generally though, I really dislike expanding the scope of variables like that. eventualy fPreferred turns into something entirely meaningless like "whitelisted"
 82 2017-10-04 20:05:15	0|jonasschnelli|Okay. Updated the PR
 83 2017-10-04 20:05:16	0|jonasschnelli|Relevant parts are here: https://github.com/bitcoin/bitcoin/pull/10387/files#diff-eff7adeaec73a769788bb78858815c91R1322
 84 2017-10-04 20:13:46	0|jonasschnelli|ping JeremyRu1in
 85 2017-10-04 20:14:21	0|jonasschnelli|My re-check / re-test told me,  rescanblockchain { start_height: 10, stop_height: 20 } does also rescan block 20
 86 2017-10-04 20:19:05	0|JeremyRubin|jonasschnelli: it doesn't work; see PR comment
 87 2017-10-04 20:19:27	0|JeremyRubin|it's the failure mode that's broken, I should have been more clear
 88 2017-10-04 20:19:47	0|JeremyRubin|but I was confused about the general range semantics too; so more clear language is good there nonetheless
 89 2017-10-04 20:27:19	0|JeremyRubin|jonasschnelli: try scanning from 10 - 20 on a healthy node
 90 2017-10-04 20:27:29	0|JeremyRubin|then, corrupt block 20 so it won't scan.
 91 2017-10-04 20:27:43	0|JeremyRubin|now, rescan again
 92 2017-10-04 20:27:46	0|JeremyRubin|same result
 93 2017-10-04 20:27:53	0|JeremyRubin|now, corrupt blocks 10-20
 94 2017-10-04 20:27:57	0|JeremyRubin|will return the same thing
 95 2017-10-04 20:28:33	0|jonasschnelli|JeremyRubin: I added a LogPrintf below the ReadBlockFromDisk and AddToWalletIfInvolvingMe scan... and block 20 was scanned.
 96 2017-10-04 20:28:44	0|jonasschnelli|But maybe I should try your setup
 97 2017-10-04 20:28:51	0|BlueMatt|jonasschnelli: will you kill me if I suggest a cleanup to nRequiredServices/nRelevantServices as a first commit in your pr?
 98 2017-10-04 20:29:15	0|JeremyRubin|jonasschnelli: Yes, if your scanning does not fail, it is fine.
 99 2017-10-04 20:29:15	0|jonasschnelli|BlueMatt: ideally we keep cleanups in a seperate PR
100 2017-10-04 20:29:19	0|BlueMatt|not like cfields ever requires it for others, but endless first-commit cleanups
101 2017-10-04 20:29:23	0|JeremyRubin|But if you have a corruption, it is not.
102 2017-10-04 20:29:23	0|jonasschnelli|BlueMatt: but I can cherry pick.. :)
103 2017-10-04 20:29:27	0|cfields|heh
104 2017-10-04 20:29:29	0|BlueMatt|jonasschnelli: let me play around a bit
105 2017-10-04 20:29:52	0|jonasschnelli|JeremyRubin: what do you mean with corruption?
106 2017-10-04 20:29:58	0|cfields|BlueMatt: only allowed if you first ack my socket changes you required :p
107 2017-10-04 20:29:59	0|jonasschnelli|invalidateblock?
108 2017-10-04 20:30:27	0|ryanofsky|jonasschnelli, if readblockfrom disk fails
109 2017-10-04 20:30:31	0|jonasschnelli|BlueMatt: yeah. Happy to pull-in a cleanup into bip159 PR
110 2017-10-04 20:30:37	0|BlueMatt|cfields: damn you
111 2017-10-04 20:31:37	0|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) ?
112 2017-10-04 20:31:45	0|cfields|I don't think that's just an implementation detail
113 2017-10-04 20:31:47	0|jonasschnelli|JeremyRubin, ryanofsky: aha... a filed ReadBlockFromDisk falsely reports a successful scan.
114 2017-10-04 20:32:05	0|jonasschnelli|cfields: Yeah. Need to add this... is unclear in the BIP
115 2017-10-04 20:32:07	0|JeremyRubin|No
116 2017-10-04 20:32:18	0|jonasschnelli|*a failed
117 2017-10-04 20:32:21	0|JeremyRubin|It correctly reports an unsuccessful scan
118 2017-10-04 20:32:29	0|jonasschnelli|yeah.. what I meant. sry
119 2017-10-04 20:32:33	0|JeremyRubin|you incorrectly interpret that result :)
120 2017-10-04 20:33:28	0|JeremyRubin|basically ANY non-nullptr return should cause you to return "scan failed"
121 2017-10-04 20:33:31	0|jonasschnelli|JeremyRubin: this would also be undetected in classic startup -rescans? Right?
122 2017-10-04 20:33:58	0|JeremyRubin|Unsure of prior semantics of rescan
123 2017-10-04 20:34:51	0|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
124 2017-10-04 20:34:57	0|jonasschnelli|the current rescan function skips corrupted blocks IMO
125 2017-10-04 20:35:40	0|JeremyRubin|Yes, but it also tells you 'no guarantees before block N'
126 2017-10-04 20:35:54	0|JeremyRubin|where N = LAST_FAILED
127 2017-10-04 20:36:18	0|JeremyRubin|this impl, however, is happy to tell you Blocks A-B succeded
128 2017-10-04 20:36:21	0|JeremyRubin|which is incorrect
129 2017-10-04 20:37:24	0|jonasschnelli|I see.. let me see how we can fix this.
130 2017-10-04 20:37:38	0|JeremyRubin|you have 2 choices I think
131 2017-10-04 20:37:42	0|jonasschnelli|I don't want to change the way how current startup -rescans work..
132 2017-10-04 20:37:46	0|JeremyRubin|either keep a vector of all failures
133 2017-10-04 20:37:58	0|jonasschnelli|aborting on a corrupted block seems not to be the ideal choice
134 2017-10-04 20:38:01	0|JeremyRubin|if passed in as a ptr arg with default nullptr
135 2017-10-04 20:38:16	0|JeremyRubin|And return a vector of failures
136 2017-10-04 20:38:31	0|jonasschnelli|or report "could not read all block" (bool)?
137 2017-10-04 20:38:53	0|JeremyRubin|That could work too...
138 2017-10-04 20:39:25	0|JeremyRubin|I would (personally) not return *anything* if there are any failures.
139 2017-10-04 20:39:45	0|jonasschnelli|AFAIK, ScanForWalletTransactions returns non-nullptr when detecting a corrupt block.. could use that
140 2017-10-04 20:41:13	0|JeremyRubin|yes, that's fine. I would return a `Maybe (Int,Int)`, and only return `Some (start, stop)` if there are no failures.
141 2017-10-04 20:42:51	0|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.
142 2017-10-04 20:43:39	0|JeremyRubin|Because asking for (1000, 2000) could fail depending on how caught up you are.
143 2017-10-04 20:44:07	0|JeremyRubin|But is not an invalid request like (2000,1000), (-10, 20), etc.
144 2017-10-04 20:45:50	0|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)
145 2017-10-04 20:48:55	0|jonasschnelli|JeremyRubin: I think that should do it: https://github.com/bitcoin/bitcoin/pull/7061/files#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceR3237
146 2017-10-04 20:50:16	0|JeremyRubin|jonasschnelli: I think that seems correct to me. If you have no failures reported, then all the blocks were scanned in that range
147 2017-10-04 20:50:50	0|jonasschnelli|Yes. If you get this (new) error, you may have scanned some of the blocks (but we won't tell which one)
148 2017-10-04 20:51:04	0|JeremyRubin|Correct.
149 2017-10-04 20:51:18	0|jonasschnelli|If you have a corrupted block you are alreafy in serious troubles... :)
150 2017-10-04 20:51:55	0|JeremyRubin|Yes. Maybe worth saying 'Rescan Failed. Potentially corruputed data files.'
151 2017-10-04 20:52:53	0|jonasschnelli|JeremyRubin: Thanks... let me add this
152 2017-10-04 20:53:18	0|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.
153 2017-10-04 20:54:07	0|jonasschnelli|JeremyRubin: can you rephrase? I don't get your range concern
154 2017-10-04 20:54:23	0|JeremyRubin|bad ranges: ()[] ([)] good ranges: [()] ok ranges: [(]) []()
155 2017-10-04 20:54:37	0|jonasschnelli|?
156 2017-10-04 20:55:02	0|JeremyRubin|(let '(' and ')' denote the start and stop range, and '[' and ']' denote genesis to tip on a number line)
157 2017-10-04 20:55:15	0|jonasschnelli|aha...
158 2017-10-04 20:55:44	0|JeremyRubin|so if i ask for (tip -10, tip+100) you should return scanned (tip-10, tip)
159 2017-10-04 20:55:47	0|jonasschnelli|stop_height is optional but start_heigt is required when stop_height is passed
160 2017-10-04 20:56:23	0|jonasschnelli|I think tip+100 should result in an error
161 2017-10-04 20:56:36	0|JeremyRubin|I don't
162 2017-10-04 20:56:48	0|JeremyRubin|because it could result from our node just being behind and syncing
163 2017-10-04 20:57:14	0|jonasschnelli|Yeah.. but would you silently ignore it? IMO it deserves that we inform the user (with an error)
164 2017-10-04 20:57:19	0|JeremyRubin|If you want it to be an error, it shouldn't be invalid it should be 'not synced to X yet'
165 2017-10-04 20:57:25	0|jonasschnelli|Then he can self-correct rather then auto-correct
166 2017-10-04 20:57:40	0|JeremyRubin|if you want it to be more clear, then you should always return the current tip
167 2017-10-04 20:58:45	0|JeremyRubin|but in the 'ok ranges' cases, the type of failure is very different from the bad ranges
168 2017-10-04 20:59:01	0|JeremyRubin|bad ranges will NEVER succeed, ok ranges will succeed if you wait long enough
169 2017-10-04 20:59:46	0|JeremyRubin|So bad ranges are invalid. ok ranges are too 'soon' for our node.
170 2017-10-04 21:01:03	0|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.
171 2017-10-04 21:02:16	0|JeremyRubin|((unless I'm not seeing some edge case? Maybe if you somehow trigger an abortrescan you would return (start, point_aborted)?))
172 2017-10-04 21:02:27	0|jonasschnelli|JeremyRubin: but with the current fix (throw when getting a non-nullptr) there is a guarantee we have scanned the given range, right?
173 2017-10-04 21:02:36	0|JeremyRubin|Correct!
174 2017-10-04 21:02:46	0|jonasschnelli|Do you see anything else t
175 2017-10-04 21:02:50	0|jonasschnelli|to fix?
176 2017-10-04 21:02:58	0|JeremyRubin|((Except see above abortrescan concern))
177 2017-10-04 21:03:14	0|jonasschnelli|abortrescan leads to an explicit error thrown
178 2017-10-04 21:03:25	0|JeremyRubin|ok -- so then no point in returning the range
179 2017-10-04 21:03:29	0|jonasschnelli|https://github.com/bitcoin/bitcoin/pull/7061/files#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceR3231
180 2017-10-04 21:03:32	0|JeremyRubin|so remove the return values
181 2017-10-04 21:03:37	0|jonasschnelli|the throw will lead to not return the range
182 2017-10-04 21:04:11	0|JeremyRubin|Actually... making abortrescan return the range seems like good UX
183 2017-10-04 21:04:52	0|jonasschnelli|We could add that later,.. seems out of scope and related to #11450
184 2017-10-04 21:04:53	0|gribble|https://github.com/bitcoin/bitcoin/issues/11450 | ScanForWalletTransactions return value is incorrectly documented · Issue #11450 · bitcoin/bitcoin · GitHub
185 2017-10-04 21:05:07	0|JeremyRubin|k -- so otherwise, you should get rid of the return values.
186 2017-10-04 21:05:17	0|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
187 2017-10-04 21:05:21	0|JeremyRubin|Unless you want to handle the ranges I've labelled 'ok'
188 2017-10-04 21:05:51	0|jonasschnelli|You are aware that a *throw" will lead to return nothing blow that acctuall throw code line?
189 2017-10-04 21:06:00	0|JeremyRubin|(which, even if not handled, I think merit a separate designation from 'invalid')
190 2017-10-04 21:06:38	0|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.
191 2017-10-04 21:07:16	0|JeremyRubin|But maybe it's better interface design to not rely on that in case we improve the behavior in the future ;)
192 2017-10-04 21:07:53	0|jonasschnelli|JeremyRubin: hm... sorry for not following completely... which absence of an error you referring to?
193 2017-10-04 21:08:12	0|jonasschnelli|Abort rescan leads to an error: https://github.com/bitcoin/bitcoin/pull/7061/files#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceR3232
194 2017-10-04 21:08:38	0|JeremyRubin|Ok. So you're a client. You ask for rescan (tip-10,tip-5).
195 2017-10-04 21:08:49	0|JeremyRubin|I'm the server.
196 2017-10-04 21:09:00	0|JeremyRubin|I see the request, and I rescan happily.
197 2017-10-04 21:09:45	0|JeremyRubin|I now will return a range (start, stop) based on your request.
198 2017-10-04 21:10:13	0|JeremyRubin|Under what circumstance do you, the caller, not already know what that range is?
199 2017-10-04 21:10:34	0|JeremyRubin|I suppose you don't know the tip if you only pass a start?
200 2017-10-04 21:11:22	0|JeremyRubin|So maybe it merits returning just the finish height, but you never don't know what start you requested already.
201 2017-10-04 21:12:54	0|JeremyRubin|Anyways... this is secondary, it's not really an issue. But I think that handling ok ranges addresses my concern anyways.
202 2017-10-04 21:13:59	0|jonasschnelli|JeremyRubin: Okay. I see you point.
203 2017-10-04 21:14:17	0|jonasschnelli|I guess we could leave it away completely then.... or keep it for future extenions...
204 2017-10-04 21:14:43	0|jonasschnelli|Although, if you keep away the stop height,... you should at leat get the height of the scanned tip
205 2017-10-04 21:14:47	0|jonasschnelli|Which may be important
206 2017-10-04 21:18:09	0|JeremyRubin|I think that handling ok ranges in the manner suggested gets this to have the correct semantics for the returned data
207 2017-10-04 21:18:12	0|jonasschnelli|JeremyRubin: thanks for reviewing and helping me to understand your point. :)
208 2017-10-04 21:18:24	0|JeremyRubin|yeah, sorry if my explanations were unclear
209 2017-10-04 21:19:14	0|JeremyRubin|It was a pretty subtle bug I guess, because a lot of people missed it!
210 2017-10-04 21:19:15	0|jonasschnelli|Re-reading your comments looks like I had my eyes covered with tomatos
211 2017-10-04 21:20:30	0|jonasschnelli|Yes. Glad you did a review.. and tell me, if you think other stuff needs to be changed/fixed.
212 2017-10-04 21:28:41	0|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')
213 2017-10-04 21:30:12	0|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).
214 2017-10-04 21:32:21	0|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).
215 2017-10-04 22:02:40	0|JeremyRubin|jonasschnelli: ^^^ afk soon, shoot an email if questions
216 2017-10-04 22:37:18	0|bitcoin-git|[13bitcoin] 15promag opened pull request #11452: Improve ZMQ functional test (06master...062017-10-improve-zmq-test) 02https://github.com/bitcoin/bitcoin/pull/11452