1 2014-10-19 00:40:38 <CodeShark> starsoccer: I invite you to try out my multisignature transaction solution: https://ciphrex.com :)
  2 2014-10-19 00:40:54 <starsoccer> CodeShark, kk I will check it out
  3 2014-10-19 00:40:55 <CodeShark> https://ciphrex.com/docs/Bitcoin_Vault-Getting_Started.pdf
  4 2014-10-19 00:40:59 <starsoccer> oo actully ive heard of this before
  5 2014-10-19 00:41:09 <starsoccer> seems like its only a download tho
  6 2014-10-19 00:41:21 <CodeShark> as opposed to a?
  7 2014-10-19 00:41:45 <CodeShark> it's 100% open source if that's what you're after
  8 2014-10-19 00:41:47 <starsoccer> well I need it to work on my site, but thanks anyways im already working on it I am almost done
  9 2014-10-19 00:42:07 <CodeShark> oh, you mean you need an API?
 10 2014-10-19 00:42:07 <starsoccer> CodeShark, that is, but I already have something thats working for me
 11 2014-10-19 00:42:09 <starsoccer> thanks anyways
 12 2014-10-19 00:42:16 <starsoccer> well api or javascript or php preferably
 13 2014-10-19 00:42:25 <starsoccer> but I already got a good javascript one working
 14 2014-10-19 04:30:42 <CodeShark> can we add the rule that main.h shall not be included in ANY source file other than main.cpp in the coding guidelines :p
 15 2014-10-19 04:31:45 <CodeShark> seriously - move all the global vars to a separate source file, add accessors to grab the values
 16 2014-10-19 04:32:25 <CodeShark> how anyone could even consider including main.h in something like rpcserver.cpp without cringing is a mystery to me
 17 2014-10-19 04:33:18 <CodeShark> I'm willing to help out in this effort - but only if I have some acks from other devs
 18 2014-10-19 05:03:05 <sipa> CodeShark: that's not real encapsulation
 19 2014-10-19 05:03:32 <CodeShark> of course not - but it's a step in that direction
 20 2014-10-19 05:03:37 <sipa> having a one file with the globals and another with all the logic dealing with them is imho much uglier
 21 2014-10-19 05:03:43 <sipa> i disagree
 22 2014-10-19 05:04:02 <sipa> move the globals into a class, move the functions into methods of that class
 23 2014-10-19 05:04:03 <CodeShark> let me be more precise
 24 2014-10-19 05:04:26 <sipa> then change visibility so all access to them goes through methods
 25 2014-10-19 05:04:58 <CodeShark> I'm not saying ALL globals should just be moved to a single file - perhaps we want to break them up into different categories of globals. we can either use singleton objects with accessor methods…or we can use static scope variables in source files and accessor functions
 26 2014-10-19 05:05:24 <sipa> the globals are just a symptom
 27 2014-10-19 05:06:07 <CodeShark> agreed - but I don't think radical changes to the codebase in a few commits is a good idea
 28 2014-10-19 05:06:13 <sipa> sure
 29 2014-10-19 05:06:25 <sipa> but moving stuff away that logically belongs together is not a solution
 30 2014-10-19 05:06:39 <sipa> we've been moving stuff out of main for a while now
 31 2014-10-19 05:06:41 <CodeShark> moving these globals outside of main.h, at the very least, makes it easier to reuse source files from bitcoind for other projects
 32 2014-10-19 05:06:49 <sipa> and encapsulating things inside classes
 33 2014-10-19 05:07:45 <sipa> i don't see how - it's not like you could use those globals without still depending on code from main
 34 2014-10-19 05:08:04 <sipa> split things up and encapsulate them
 35 2014-10-19 05:14:13 <CodeShark> well, some variables do depend critically on details of implementations - such as cvBlockChange (ugh)
 36 2014-10-19 05:14:46 <CodeShark> others are simply flags - some of which remain unchanged for the whole duration of execution
 37 2014-10-19 05:15:38 <CodeShark> with the latter, the solution is simply to pass the flags to objects when instantiating them
 38 2014-10-19 05:16:41 <CodeShark> for flags that can change, we can use some sort of signaling mechanism
 39 2014-10-19 05:18:11 <CodeShark> as for global state, we should use singleton objects with accessors, like you said
 40 2014-10-19 05:21:55 <CodeShark> we should definitely be avoiding things like LOCK(cs_main) in rpcserver.cpp. Instead, the specific operations on things like the wallet should be protected inside the wallet class itself
 41 2014-10-19 05:25:49 <CodeShark> it's kinda sad, actually - most of the work has been done to implement what could be a generally useful http server that accepts json-rpc commands…but it's impossible to use it in any other projects as the code is now
 42 2014-10-19 06:02:21 <CodeShark> HttpJsonRpcServer server(username, password, allowed ips, ssl info, etc….);
 43 2014-10-19 06:03:00 <CodeShark> server.addCommand("newcommand", &commandFunc, parameterBounds);
 44 2014-10-19 06:03:08 <CodeShark> server.start();
 45 2014-10-19 06:03:11 <CodeShark> and voila :)
 46 2014-10-19 06:04:20 <CodeShark> a fully functioning HTTP server that can handle JSON-RPC requests in three lines :)
 47 2014-10-19 06:07:29 <CodeShark> at least that's how I would do it :p
 48 2014-10-19 07:03:00 <wumpus> CodeShark: I disagree there too; X.h is the interface file for X.cpp; so anything that uses functions or variables from X is supposed to include X.h ... that main.h is a tangled mess with P2P and consensus code interleaved is something that needs to be solved, but we're moving in that way
 49 2014-10-19 07:03:23 <wumpus> CodeShark: but including main.h is not a problem in itself that needs to be solved
 50 2014-10-19 07:03:53 <CodeShark> the inclusion of main.h is a symptom - not the cause of the problem
 51 2014-10-19 07:04:34 <CodeShark> the overriding issue in my mind is the fact that a lot of work has been put into building units that could be eminently useful in other projects - but cannot be pulled out of bitcoind without breaking everything
 52 2014-10-19 07:05:01 <CodeShark> so this work is essentially "wasted"
 53 2014-10-19 07:05:33 <CodeShark> the http server stuff is one prominent example
 54 2014-10-19 07:05:45 <wumpus> something specific such as detangling the http server from main.h is fine with me
 55 2014-10-19 07:06:19 <wumpus> but don't go all 'let's move all globals around!'.. globals belong with the code that uses them, if possible they shouldn't be exposed outside their module at all
 56 2014-10-19 07:07:43 <CodeShark> I don't disagree - I didn't mean to give that impression
 57 2014-10-19 07:08:40 <CodeShark> there's also a secondary issue here, though, which is that changing even a single character in main.h means an enormous portion of the project needs to be rebuilt from scratch
 58 2014-10-19 07:09:05 <wumpus> that's another problem that stems from the fact that main.h is a tangled with with too many responsibilities
 59 2014-10-19 07:09:17 <wumpus> I think there are some pulls around that peel responsibilities from main
 60 2014-10-19 07:09:50 <CodeShark> and a third issue is the higher likelihood of contention between developers and difficulty of merging
 61 2014-10-19 07:09:54 <wumpus> it's not *completely* trivial though, it's easy to introduce modules with circular dependencies for example
 62 2014-10-19 07:11:23 <CodeShark> the circular dependencies might be dealt with by decoupling interfaces
 63 2014-10-19 07:11:28 <wumpus> yes, we know
 64 2014-10-19 07:11:39 <wumpus> that's what happened with net and main as well
 65 2014-10-19 07:11:51 <wumpus> decoupling using boost::signal etc
 66 2014-10-19 07:13:26 <wumpus> so let's be specific: why does rpcserver need main.h?
 67 2014-10-19 07:13:47 <wumpus> ... I honestly can't think of a reason
 68 2014-10-19 07:13:55 <CodeShark> it really shouldn't
 69 2014-10-19 07:14:00 <wumpus> but why does it?
 70 2014-10-19 07:14:03 <wumpus> what does it use from there?
 71 2014-10-19 07:14:05 <CodeShark> it currently accesses synchronization objects
 72 2014-10-19 07:14:27 <CodeShark> cs_main and cvBlockChanged
 73 2014-10-19 07:14:33 <wumpus> right - the cs_main / cs_wallet
 74 2014-10-19 07:14:59 <wumpus> because CRPCTable::execute contains bitcoin specific logic
 75 2014-10-19 07:15:18 <wumpus> if the RPC table entries would 1) take care of locking themselves or 2) be wrapped in something that takes care of locking
 76 2014-10-19 07:15:28 <CodeShark> yes, indeed - and the locks are probably at the wrong level in the call stack
 77 2014-10-19 07:15:31 <wumpus> this problem would go away
 78 2014-10-19 07:16:00 <wumpus> but this is convenience for us, mainly: from the table it's possible to see whether locks are necessary
 79 2014-10-19 07:16:01 <CodeShark> the http server should just serve to dispatch commands generally
 80 2014-10-19 07:16:16 <wumpus> there used to be a huge number of locking issues in RPC, so a sledgehammer approach was taken
 81 2014-10-19 07:16:21 <CodeShark> the called functions should handle any application specific issues
 82 2014-10-19 07:17:16 <wumpus> the problem there is that *every* RPC function would need to be audited, or at least the locking added
 83 2014-10-19 07:17:42 <CodeShark> why?
 84 2014-10-19 07:17:53 <wumpus> something that needs to be done at some point anyway for efficiency, but then again, as long as almost everything is behind the same cs_main lock it makes little sense to not do it this way...
 85 2014-10-19 07:18:04 <CodeShark> the locking shouldn't take place at the call stack level of the http server command dispatching
 86 2014-10-19 07:18:14 <wumpus> or something like a a CRPCCommand subclass that does the locking and then calls the function
 87 2014-10-19 07:19:29 <wumpus> (so split rpcserver into a rpcserver generic and bitcoin specific module)
 88 2014-10-19 07:19:37 <CodeShark> yep
 89 2014-10-19 07:19:51 <wumpus> anyhow - I doubt other people would want to use our rpcserver anyhow, so this is mostly an academic issue
 90 2014-10-19 07:19:58 <CodeShark> I want to use it :)
 91 2014-10-19 07:19:59 <wumpus> it's not like it that great
 92 2014-10-19 07:20:00 <wumpus> ok?
 93 2014-10-19 07:20:06 <CodeShark> no, it's not that great - but it works
 94 2014-10-19 07:21:08 <CodeShark> right now I'm debating whether it's easier to clean it up to make it generic - or whether to implement another http json-rpc server
 95 2014-10-19 07:21:19 <wumpus> jgarzik has a pretty nice one around somewhere
 96 2014-10-19 07:21:27 <wumpus> one that is really asynchronous
 97 2014-10-19 07:21:28 <CodeShark> in c++?
 98 2014-10-19 07:21:30 <CodeShark> or c?
 99 2014-10-19 07:21:30 <wumpus> yes
100 2014-10-19 07:21:48 <wumpus> it's meant as a drop in replacement for ours, but we just never got around to it
101 2014-10-19 07:21:55 <CodeShark> ah, neat
102 2014-10-19 07:22:03 <CodeShark> do you know where it is?
103 2014-10-19 07:23:16 <wumpus> https://github.com/jgarzik/rpcsrv
104 2014-10-19 07:23:25 <CodeShark> yeah, just found it too :)
105 2014-10-19 07:23:33 <CodeShark> you beat me to pasting the link
106 2014-10-19 07:25:00 <CodeShark> cool, thanks - I'll have a look
107 2014-10-19 07:25:04 <CodeShark> wow - 3 years ago :)
108 2014-10-19 07:29:08 <CodeShark> there's no LICENSE file - do you know if it's all boost licensed?
109 2014-10-19 07:29:43 <CodeShark> I guess it is
110 2014-10-19 07:30:05 <wumpus> I don't know
111 2014-10-19 07:30:24 <CodeShark> it's apparently an adaptation of a boost example
112 2014-10-19 07:30:24 <wumpus> eh there is a COPYING - it says Boost indeed
113 2014-10-19 07:32:06 <wumpus> yes
114 2014-10-19 07:37:08 <CodeShark> you're right - salvaging the http server that's in bitcoind might not be worth it :)
115 2014-10-19 07:39:24 <CodeShark> as for the threadsafe stuff and the locks, just place the lock inside the functions that require them :)
116 2014-10-19 07:39:29 <wumpus> as I said it's not really a great way of doing things, the only advantage of using bitcoind's approach would be that it is time-tested code
117 2014-10-19 07:40:31 <CodeShark> instead of if (pcmd->threadsafe) …, just do LOCK(…) inside pcmd->actor()
118 2014-10-19 07:40:37 <wumpus> it used to be that way
119 2014-10-19 07:40:48 <CodeShark> why isn't it anymore?
120 2014-10-19 07:40:50 <wumpus> at some point (long ago) it was changed to this to give a good overview in the command table
121 2014-10-19 07:41:05 <CodeShark> doh!
122 2014-10-19 07:41:05 <wumpus> easier for auditing the locks etc
123 2014-10-19 07:41:22 <sipa> well, locks should be pushed down
124 2014-10-19 07:41:29 <sipa> into the RPC methods themselves
125 2014-10-19 07:41:30 <wumpus> as said there used to be tons of problems with RPC locking - there have been none after that
126 2014-10-19 07:41:48 <sipa> and ideally, into the core code that accesses the actual data structures
127 2014-10-19 07:42:02 <wumpus> sure, that would be the ideal case, just lock where the actual main structures are used
128 2014-10-19 07:42:21 <sipa> yeah, but right now, I see no reason why we wouldn't just push down all locks
129 2014-10-19 07:43:12 <sipa> the table is a replacement for just an uncondition LOCK(cs_main, cs_wallet) that was done around every rpc call
130 2014-10-19 07:43:30 <sipa> which itself was added to fix a lot of locking issues with the RPC code (in 0.3.x days)
131 2014-10-19 07:43:59 <sipa> i mean: an unconditional lock that was done before calling the rpc method
132 2014-10-19 07:44:08 <wumpus> but the obvious approach would have been to move the LOCKs into  each of the rpc methods themselves
133 2014-10-19 07:44:28 <sipa> yeah, i think we should do that
134 2014-10-19 07:44:40 <sipa> as that will make it clear where we could push them down further
135 2014-10-19 07:44:46 <wumpus> so somehow the choice was made to use a table instead
136 2014-10-19 07:45:48 <CodeShark> the further down the call stack we can place the locks (without creating race conditions) the better :)
137 2014-10-19 07:46:09 <sipa> indeed
138 2014-10-19 07:46:14 <wumpus> I think that was done for the above reason, to make it easier to see in one glance what does locks and what does not, but I don't care, if everyone agrees with pushing them in to every RPC function that's fine with me...
139 2014-10-19 07:46:50 <wumpus> it was also before the wallet/main split; putting locks into every function could be more granular, the non-wallet functions wouldn't have to lock the wallet
140 2014-10-19 07:46:55 <sipa> yeah, maybe it's clearer this way, but it's very limited
141 2014-10-19 07:47:07 <sipa> locking should be done at a much more granular level than full rpcs
142 2014-10-19 07:47:32 <wumpus> but only if that can be done without introducing issues, I don't look forward to bringing back RPC locking issues :)
143 2014-10-19 07:47:57 <CodeShark> it seems the logical issues have been sorted out, at least at the rpc function level
144 2014-10-19 07:47:58 <sipa> we can be pretty conservative still
145 2014-10-19 07:48:01 <CodeShark> right
146 2014-10-19 07:48:36 <wumpus> ok, agreed then
147 2014-10-19 07:49:11 <wumpus> that leaves cvBlockChange
148 2014-10-19 07:49:40 <wumpus> it's weird - why is there a cvBlockChange.notify_all() in the RPC code?
149 2014-10-19 07:49:46 <sipa> no clue
150 2014-10-19 07:50:10 <CodeShark> mining code
151 2014-10-19 07:50:11 <CodeShark> lol
152 2014-10-19 07:50:24 <sipa> no
153 2014-10-19 07:50:24 <wumpus> oh it's like satoshi's random sleeps in the shutdown code
154 2014-10-19 07:50:41 <wumpus> just a leftover
155 2014-10-19 07:50:43 <sipa> no
156 2014-10-19 07:51:11 <CodeShark> getblocktemplate
157 2014-10-19 07:51:51 <CodeShark> hmmm
158 2014-10-19 07:52:21 <sipa> cvBlockChange is for long polling, and there it makes perfect sense
159 2014-10-19 07:52:23 <wumpus> right, to wake up getblocktemplate at shutdown
160 2014-10-19 07:52:40 <sipa> and you need to make sure it wakes up so it can return before shutting down
161 2014-10-19 07:52:56 <CodeShark> couldn't this be accomplished with some signal?
162 2014-10-19 07:52:58 <wumpus> but it should be part of RPC logic, not of main
163 2014-10-19 07:53:03 <CodeShark> rather than accessing the condition variable directly?
164 2014-10-19 07:53:19 <sipa> the condition variable is the single
165 2014-10-19 07:53:24 <sipa> it's only there for one purpose
166 2014-10-19 07:53:30 <sipa> *singal
167 2014-10-19 07:53:36 <sipa> not using it would mean polling
168 2014-10-19 07:53:38 <wumpus> yes, it could be decoupled by having main call a boost::signal
169 2014-10-19 07:53:39 <CodeShark> the RPC server sends a "shutting down" signal, then anyone listening can do whatever is needed to clean up
170 2014-10-19 07:53:50 <wumpus> TipUpdated
171 2014-10-19 07:54:13 <wumpus> RPC logic would then subscribe to that
172 2014-10-19 07:54:21 <CodeShark> exactly
173 2014-10-19 07:54:24 <sipa> wumpus: right, the condition variable could be in purely in RPC, and the singal handler from main in RPC could notify
174 2014-10-19 07:54:28 <sipa> perfecgt
175 2014-10-19 07:54:29 <sipa> go for it
176 2014-10-19 07:55:17 <sipa> wumpus: what was the plan for the license change thing?
177 2014-10-19 07:55:17 <wumpus> and yes ther eould he a RPCServerShuttingDown signal in rpcserver, that is subscribed to by the same initialization code, which also notifies the condition variable
178 2014-10-19 07:55:23 <wumpus> sipa: everything MIT
179 2014-10-19 07:55:56 <sipa> wumpus: yes, but I thought we'd only do that for new code
180 2014-10-19 07:56:00 <wumpus> get rid of /X11 in the codebase
181 2014-10-19 07:56:10 <sipa> now Diapolo is nagging everyone who changing code somewhere to also change the license
182 2014-10-19 07:56:24 <sipa> if the plan is to change it everywhere, let's do it at once and be done with it
183 2014-10-19 07:56:26 <wumpus> well for new code it's most important to not introduce the confusion in the first place
184 2014-10-19 07:56:41 <wumpus> not sure why he's nagging for changes to *existing* code, that makes no sense
185 2014-10-19 07:56:54 <sipa> well, code moved to new files
186 2014-10-19 07:57:08 <wumpus> ok well in that case he's right: no /X11 should appear in new files
187 2014-10-19 07:57:26 <wumpus> I'm fine with doing it at once thoug
188 2014-10-19 07:57:50 <sipa> ah, i thought the reason for not doing it was because we aren'ts exactly sure whether we're not changing anything
189 2014-10-19 07:57:56 <sipa> so i used it for new _code_
190 2014-10-19 07:58:01 <sipa> but not for old code in new files
191 2014-10-19 07:58:22 <wumpus> all files contain a mix of old and new code, that would be too complex
192 2014-10-19 07:58:38 <sipa> let's then be done with it and change it everywhere
193 2014-10-19 07:58:46 <wumpus> fine with that
194 2014-10-19 08:00:00 <wumpus> better that than to nag everyone every time
195 2014-10-19 08:00:53 <wumpus> which is annoying about diapolo anyhow; no matter what the pull, the first lead of comments is always lots of busywork about extra spaces, sorting of include files, etc...
196 2014-10-19 08:01:35 <sipa> yeah, i care about such rules, but fixing it occasionally all over the place is easier
197 2014-10-19 08:01:51 <wumpus> well I'm afraid it distracts from real in-depth review
198 2014-10-19 08:02:05 <wumpus> it's easy to brush off nonsensical comments, then it's easy to forget about a real concern
199 2014-10-19 08:03:43 <wumpus> so I'm going to remove the rule about sorting includes from the coding style, it gives him less ammo
200 2014-10-19 08:05:05 <wumpus> (as it's one of the things clang-format won't take care of)
201 2014-10-19 08:11:05 <CodeShark> there's only a couple other minor issues in rpcserver
202 2014-10-19 08:11:12 <CodeShark> GetRandBytes and GetWarnings
203 2014-10-19 08:13:01 <CodeShark> actually, just GetWarnings
204 2014-10-19 08:13:14 <CodeShark> the first one is just random.h
205 2014-10-19 08:50:12 <CodeShark> I don't quite get why some of the RPC methods are not considered threadsafe - but I'll be extremely conservative and just follow the table
206 2014-10-19 08:50:44 <sipa> CodeShark: which ones in particular?
207 2014-10-19 08:50:45 <Diablo-D3> http://cursors.io/
208 2014-10-19 08:50:53 <CodeShark> well, for instance, decoderawtransaction
209 2014-10-19 08:51:21 <sipa> should be threadsafe
210 2014-10-19 08:51:26 <CodeShark> yeah, one would think
211 2014-10-19 08:51:51 <sipa> it is
212 2014-10-19 08:51:52 <CodeShark> I figure I'll do this in a few steps - the first step is just to move the locks down the call stack
213 2014-10-19 08:51:55 <sipa> seems like an oversight
214 2014-10-19 08:52:13 <sipa> yup
215 2014-10-19 08:52:35 <CodeShark> the only minor issue is rpc methods that call other rpc methods
216 2014-10-19 08:52:43 <CodeShark> which means we must unlock before calling
217 2014-10-19 08:52:53 <sipa> why?
218 2014-10-19 08:52:58 <sipa> abstract the common functionality out
219 2014-10-19 08:53:14 <CodeShark> well, that's not part of the first step :)
220 2014-10-19 08:53:16 <sipa> ok
221 2014-10-19 08:53:25 <sipa> but they're recursive locks
222 2014-10-19 08:53:32 <sipa> you can lock while you already have the lock
223 2014-10-19 08:53:38 <CodeShark> oh, rly?
224 2014-10-19 08:53:44 <sipa> which is bad practice, but very convenient sometimes
225 2014-10-19 08:53:47 <sipa> yes
226 2014-10-19 08:53:55 <sipa> they're less efficient than non-recursive ones, though
227 2014-10-19 08:54:22 <CodeShark> the best solution is the one you mentioned - pull out common functionality, write locking wrappers around the ones exposed externally
228 2014-10-19 08:54:26 <sipa> yup
229 2014-10-19 08:58:22 <CodeShark> also, for step 1 I'm just placing the lock right after the help exceptions
230 2014-10-19 08:58:36 <CodeShark> even if we don't get into contention until several lines further down
231 2014-10-19 08:58:58 <CodeShark> to make it easy to verify they do follow the table
232 2014-10-19 08:59:20 <CodeShark> then we can continue moving them down and restricting them to the actual contentious lines of code
233 2014-10-19 09:24:42 <CodeShark> ok, step 1 is almost complete - just need to do the signal thing for getblocktemplate
234 2014-10-19 09:24:43 <CodeShark> https://github.com/CodeShark/bitcoin/tree/No_main_in_rpcserver
235 2014-10-19 09:25:18 <CodeShark> hmm, the makefiles shouldn't have gotten in there
236 2014-10-19 09:27:30 <CodeShark> ok, fixed - ignore my last statement
237 2014-10-19 09:28:30 <CodeShark> ok, so two more things to do - the signal for cvBlockChange - and GetWarnings
238 2014-10-19 09:28:42 <CodeShark> not 100% sure what the best thing to do is for the latter
239 2014-10-19 09:51:41 <wumpus> CodeShark: there should be a signal that allows breaking off RPC commands before they're executed, this will also be necessary for  #5007
240 2014-10-19 09:54:13 <CodeShark> hmm...
241 2014-10-19 09:54:44 <wumpus> both are cases of 'modes' where RPC is (partially) disabled
242 2014-10-19 09:54:54 <CodeShark> I'm thinking this is probably not something that should be handled in rpcserver but in the RPC methods
243 2014-10-19 09:55:18 <CodeShark> we're already checking parameters and a "help" mode
244 2014-10-19 09:55:20 <wumpus> no, this should not be handled in every method indivdually, especially not the warmup mode
245 2014-10-19 09:55:41 <wumpus> the idea of the blanket block is to make sure that nothing sneaks through
246 2014-10-19 09:56:45 <CodeShark> it's easiest to do it when calling actor() - but I don't like the idea of an added conditional there
247 2014-10-19 09:57:51 <CodeShark> and turning off warmup mode could be done directly with a function call as long as we avoid a circular dependency
248 2014-10-19 09:57:59 <wumpus> just add a signal that gets executed on every RPC command
249 2014-10-19 09:58:12 <CodeShark> oh, I get what you're saying
250 2014-10-19 09:58:26 <wumpus> it can raise an exception when a) safe mode b) warmup mode
251 2014-10-19 09:58:27 <CodeShark> yeah, that could work I suppose
252 2014-10-19 09:59:24 <CodeShark> a slot that returns a status flag of some sort
253 2014-10-19 09:59:34 <wumpus> it doesn't even need to return anything
254 2014-10-19 09:59:52 <wumpus> it can either throw an JSONRPCError() or not
255 2014-10-19 10:00:02 <CodeShark> ah, yes - that too :)
256 2014-10-19 10:00:06 <CodeShark> we wrap it in the try clause
257 2014-10-19 10:00:08 <wumpus> returning a value is annoying
258 2014-10-19 10:00:11 <CodeShark> before calling action()
259 2014-10-19 10:00:23 <wumpus> (from a slot) you have to handle combining multiple values etc
260 2014-10-19 10:00:24 <CodeShark> yeah, that's better - you're right
261 2014-10-19 10:01:11 <CodeShark> alright - I think we're on the same page, then :)
262 2014-10-19 10:02:17 <CodeShark> how to go about fixing GetWarnings?
263 2014-10-19 10:02:30 <wumpus> 'fixing'?
264 2014-10-19 10:02:37 <CodeShark> GetWarnings is implemented in main.cpp
265 2014-10-19 10:02:49 <CodeShark> it gets called from rpcserver.cpp
266 2014-10-19 10:03:22 <wumpus> register a slot that does the GetWarnings check (in rpc server bitcoin specific) to the PreRpcCall() signal
267 2014-10-19 10:04:00 <wumpus> that frees the general RPC server from bitcoin-specific logic, and it doesn't have to care about GetWarnings anymore
268 2014-10-19 10:04:19 <CodeShark> ah, part of the same solution
269 2014-10-19 10:04:40 <CodeShark> yeah - lol
270 2014-10-19 10:04:42 <wumpus> yes, i suppose it can use the same solution, although it will have to pass the RPC command name and be a bit more selective
271 2014-10-19 10:05:05 <wumpus> or RPC command object, whatever makes sense
272 2014-10-19 10:05:17 <CodeShark> not sure I follow
273 2014-10-19 10:05:27 <wumpus> well the safe mode is more selective than warmup mode
274 2014-10-19 10:05:36 <wumpus> warmup mode disables *all* RPC calls, so that's just a throw ...
275 2014-10-19 10:05:40 <CodeShark> in CRPCTable::execute, we're calling GetWarnings("rpc")
276 2014-10-19 10:05:47 <wumpus> safe mode allows some RPC calls, but disables others
277 2014-10-19 10:05:55 <CodeShark> ah, ok - I see
278 2014-10-19 10:06:01 <wumpus> so the PreRPCCall() signal has to know *which* one
279 2014-10-19 10:06:29 <CodeShark> why not just have it pass some flag which can be interpreted appropriately by the slot?
280 2014-10-19 10:06:47 <CodeShark> no need for the slot to know about the table structure
281 2014-10-19 10:06:47 <wumpus> oh that's possible too, depends on how you want to split the functionality
282 2014-10-19 10:07:06 <CodeShark> although...
283 2014-10-19 10:07:10 <wumpus> but the slot has knowledge about what safe mode is (bitcoin specific knowledge)
284 2014-10-19 10:07:15 <wumpus> the caller has not
285 2014-10-19 10:07:34 <CodeShark> ideally we want to move the table out of rpcserver as well - but it can still be a structure defined in rpcserver but initialized elsewhere
286 2014-10-19 10:07:43 <wumpus> yes, the table should move eventually
287 2014-10-19 10:07:56 <wumpus> it could even be split up at some point, so that modules can register their own methods
288 2014-10-19 10:08:05 <CodeShark> right, that's what I had in mind
289 2014-10-19 10:08:13 <wumpus> it's bitcoin specific anyhow, so it belongs with bitcoin specific code
290 2014-10-19 10:08:18 <wumpus> not in the general RPC server
291 2014-10-19 10:08:58 <wumpus> for ex. the RPC server would just provide an interface to register methods, the bitcoin specific code would go through the table and register the methods
292 2014-10-19 10:09:23 <CodeShark> we could just associate some flag of sufficient width with each method whose exact format can be defined by the bitcoin-specific code
293 2014-10-19 10:09:53 <CodeShark> or even a template to support arbitrary types
294 2014-10-19 10:10:02 <wumpus> let's try not to overdesign
295 2014-10-19 10:10:12 <CodeShark> ok, then just a uint64_t :;
296 2014-10-19 10:10:34 <CodeShark> 64 boolean flags should be more than enough :p
297 2014-10-19 10:10:48 <wumpus> maybe: make the application-specific module subclass CRPCCommand, and  it could add fields as necessary
298 2014-10-19 10:11:15 <CodeShark> hmm, perhaps
299 2014-10-19 10:11:32 <CodeShark> and then do an explicit typecast in the slot?
300 2014-10-19 10:12:10 <wumpus> ... but if we're subclassing CRPCCommand
301 2014-10-19 10:12:16 <wumpus> why not add the checks to execute :p
302 2014-10-19 10:12:18 <wumpus> hmm
303 2014-10-19 10:14:06 <wumpus> so the RPC server just calls the virtual method execute(), and the application-specific code does any checks, locking that it deems needed before dispatching further
304 2014-10-19 10:16:02 <CodeShark> right - essentially the callable model
305 2014-10-19 10:16:15 <CodeShark> we could just use operator() :)
306 2014-10-19 10:16:17 <wumpus> it seems less contorted than passing around an object that will be passed back and interpreted arbitrarily anyhow
307 2014-10-19 10:16:21 <wumpus> yes
308 2014-10-19 10:17:25 <CodeShark> so we construct a map from string method names to callable objects
309 2014-10-19 10:18:48 <wumpus> that would be the easiest interpretation, yes
310 2014-10-19 10:18:57 <CodeShark> we can subclass the base callable object for safemode, etc...
311 2014-10-19 10:20:01 <CodeShark> and we provide a method for registering a new command
312 2014-10-19 10:20:06 <wumpus> SafeModeSafeRPCCommand :p
313 2014-10-19 10:20:10 <CodeShark> :)
314 2014-10-19 10:20:30 <CodeShark> then each rpc unit could register its own methods
315 2014-10-19 10:20:32 <wumpus> (well, more likely the other way around, as checking for safe mode adds code...)
316 2014-10-19 10:20:52 <wumpus> yes - but I'd leave that for later
317 2014-10-19 10:21:41 <wumpus> but indeed in principle rpc*.cpp could register their own methods
318 2014-10-19 10:22:24 <wumpus> but in the beginning it's better to restrict the scope to splititng rpcserver into a generic and bitcoin specific part
319 2014-10-19 10:22:37 <CodeShark> yeah, I'm all for the incremental approach
320 2014-10-19 10:22:58 <CodeShark> the fewer source files touched per commit, the easier to review
321 2014-10-19 10:23:10 <wumpus> right
322 2014-10-19 10:24:04 <wumpus> and probably start by moving the locking down, as that's most straightforward
323 2014-10-19 10:24:58 <CodeShark> so in the spirit of this incremental approach, I propose we first continue to use the table along with the safemode flag, replace GetWarnings with a signal, and fix the cvBlockChange thing
324 2014-10-19 10:25:15 <CodeShark> get the damn thing to build, test it
325 2014-10-19 10:25:18 <wumpus> yes
326 2014-10-19 10:25:19 <CodeShark> merge
327 2014-10-19 10:25:24 <CodeShark> and then we do what you're saying
328 2014-10-19 10:28:31 <wumpus> the flags in the table would go away one by one; threadSafe is a matter of pushing the locks into the functions, okSafeMode would be handled by a the RPCCommand() subclass, reqWallet could be replaced by only registering wallet functions when the wallet is compiled in and active
329 2014-10-19 10:29:19 <CodeShark> we're no longer using the threadSafe flag here https://github.com/CodeShark/bitcoin/tree/No_main_in_rpcserver
330 2014-10-19 10:29:58 <CodeShark> and I don't think we're using reqWallet either
331 2014-10-19 10:30:45 <wumpus> right, I haven't looked at that code yet
332 2014-10-19 10:31:51 <CodeShark> I commented out the GetWarnings and cvBlockChange.notify_all() to make sure it builds
333 2014-10-19 10:32:27 <wumpus> going to do that tomorrow, if you don't mind :)
334 2014-10-19 10:32:41 <CodeShark> so just fixing these two things for step 1 and we're ready to merge
335 2014-10-19 10:33:18 <CodeShark> no rush :)
336 2014-10-19 10:33:33 <CodeShark> I should probably call it a night soon
337 2014-10-19 10:59:18 <dagan> Please im looking to integrate bitcoin into our webapp. Launchlab.me I want to know are there any issues with using angular?
338 2014-10-19 13:03:13 <jgarzik> sipa, wumpus: The CRPCCommand table includes locks as a temporary step towards lock pushdown.  It enabled some RPCs to run lock-free.  Removing the 'lock' column from the RPC table was always a goal.
339 2014-10-19 13:03:27 <wumpus> jgarzik: ok, great
340 2014-10-19 13:03:28 <jgarzik> so +1 on lock pushdown
341 2014-10-19 14:04:02 <kyuupichan> ic.  Perhaps an elaborated version command is due?
342 2014-10-19 14:04:02 <kyuupichan> Sorry if it's a stupid question, but should a headers-first implementation be sending its header-height in the version command, or it's confirmed height?  I can see both being useful - a headers-only implementation is not good for asking for blocks at the full height before its own download and verification has happened, but it's perfectly capable of supplying headers to other nodes, so reporting 0 height seems overly pessimist
343 2014-10-19 15:19:24 <wumpus> kyuupichan: I think we send the confirmed height, but in the end I don't think it matters;  the height in the version command is all but ignored, it's just an indication
344 2014-10-19 15:21:13 <wumpus> we don't use it anymore for anything in bitcoin core as any use of it is insecure (and there are ways to get either height in a more reliable way)
345 2014-10-19 23:52:33 <kyuupichan> wumpus: Thanks
346 2014-10-19 23:56:48 <Luke-Jr> sipa: ActivateBestChainStep, if (!ConnectTip(state, pindexConnect, pindexConnect == pindexMostWork ? pblock : NULL)) {
347 2014-10-19 23:56:57 <Luke-Jr> sipa: state is assumed to be "clean" here, rigth?