Add getblockbyheight method / support @height in place of blockhash for getblock etc (
rpc) Aug 7, 2019
getblock RPC method takes a block hash as its first argument.
Users often want to get the block details for a block at a specific height, and so need to run
getblockhash <height> and then
getblock <hash> with the hash returned.
There have been several user requests and PRs to change the
getblock RPC to accept a block height and return information about that block at that height in the main chain.
PR 16345 adds a new RPC method
getblockatheight, which duplicates the logic in
getblock, but takes a height instead of a block hash as its first argument.
PR 16317 instead allows all RPC methods arguments that take a block hash to instead take a block height when prefixed with an
getblock @123456 would get information about the block at height 123456).
What other solutions have been proposed to this user request?
What are the problems with overloading RPC arguments to be interpreted as different types?
What test cases have been added to these two PRs? What additional test cases could be added?
Which approach do you like best?
4 13:00 <emilengler> Does it start now :)
9 13:01 <digi_james> Hello ..
10 13:01 <emilengler> Oh that is my pull request
12 13:01 <jnewbery> Welcome everyone! I'll start with my normal reminder: The point of the review club is to give participants the tools and knowledge they need to take part in the Bitcoin Core review process on github.
13 13:02 <jnewbery> This meeting is for you to ask any questions you want about the review process, so you can test/review/leave comments on github.
15 13:03 <jnewbery> it's got a bunch of tools for helping with Bitcoin Core review. Might be worth taking a look at.
16 13:03 <jnewbery> ok, let's get started
17 13:04 <jnewbery> from the notes: "Users often want to get the block details for a block at a specific height, and so need to run getblockhash <height> and then getblock <hash> with the hash returned."
18 13:04 <jnewbery> First question: What other solutions have been proposed to this user request?
19 13:04 <emilengler> Two PRs
20 13:04 <emilengler> And others which overload arguments at the getblock method
21 13:04 <Emzy> To use a prefix for the hight
22 13:04 <jkczyz> Use parameter type; string for hash and integer for height
23 13:05 <emilengler> Like getblock 42 to get block 42 or getblock HASHOFBLOCK42
24 13:05 <jnewbery> any others?
25 13:05 <fjahr> 1. plain overloading 2. overloading with prefix 3. new rpc call
26 13:05 <emilengler> jnewbery: iirc no
27 13:05 <jkczyz> JSON RPC promiose pipelinig
28 13:06 <jnewbery> jkczyz: yes, that was proposed by wumpus I think
30 13:06 <lightlike> also leave as is and leave it up to the users to create aliases etc. locally
31 13:06 <jkczyz> yep, to avoid the round trip but still define the interface in terms of primitives
32 13:07 <jnewbery> ok, next question: What are the problems with overloading RPC arguments to be interpreted as different types?
33 13:07 <emilengler> It might be interpreted wrong
34 13:07 <jkczyz> Might be ambiguous as to whether the user intended a height or hash
35 13:08 <Emzy> Introducing parsing bugs.
36 13:08 <emilengler> especially in plain overloading
37 13:08 <jnewbery> jkczyz: right. The block hash is in hex, so it's possible (although unlikely) that every hex character is a digit, and the RPC would interpret it as a height
38 13:08 <jnewbery> Emzy: yes, it makes parsing logic more complex
39 13:08 <PaulTroon> script could fail in a way that gives bad input and then call returns wrong value instead of error
40 13:08 <emilengler> This wouldn't happen with a prefix character
41 13:09 <hanh> Changing a widely-used interface may break software depending on it.
42 13:09 <jkczyz> parameter names could be confusing if named as hash but given a height
44 13:10 <jnewbery> There is already one RPC that overloads an argument in this way. Does anyone know which one?
45 13:10 <jnewbery> actually more than one
48 13:12 <emilengler> Is a hash a num?
49 13:12 <jnewbery> one issue with these is that it makes type-checking a bit messier
50 13:12 <jnewbery> emilengler: hash is always a string in RPC methods
51 13:12 <digi_james> How is parsing ambuity solved with getblockstats?
52 13:12 <emilengler> But why is it RPCARG::Type::NUM then
54 13:14 <jnewbery> emilengler: To allow the case where a block height is passed
55 13:14 <achow101> range in importmulti can also be multiple types
56 13:14 <achow101> as can timestamp
57 13:14 <emilengler> jnewbery: But why is it a num then?
58 13:14 <emilengler> This is also a problem of overloading
59 13:14 <emilengler> Could be problematic at autocomplete
60 13:15 <emilengler> And how do you transmit the hash? It is just weird if params:["hash"]
61 13:15 <jnewbery> yeah - overloading can make client code more complex
62 13:16 <emilengler> And why is getblockstats using a NUM and not a STR
63 13:16 <emilengler> A string can contain ASCII numbers which can be converted to real numbers then
64 13:16 <emilengler> But vice versa it is a bit more complicated
65 13:16 <emilengler> So a STR would be probably a better choice then
66 13:17 <emilengler> And there we have the problems of overloading...
68 13:18 <Emzy> Better because no dublicated code.
69 13:18 <jkczyz> I'd prefer it to be done at the client side rather than server side
70 13:19 <PaulTroon> would prevent accidental entry interpreted as height
71 13:19 <lightlike> it is nice that is solves the problem for many RPCs, not just for getblock
72 13:19 <jnewbery> Emzy: yeah, I think the duplicate code is a problem in 16345. It could be re-implemented with less duplicate code though
73 13:19 <emilengler> Worser because overloading which I don't think is agood approach
74 13:19 <jkczyz> While the extra round trip is not desirable, once you get a block you can get the next and previous hash easily
75 13:20 <emilengler> Maybe it is also causing backwards comptabitlity issues
76 13:20 <jnewbery> jkczyz: what do you think about ryanofsky's comment "Can someone be specific about an actual advantage that would come from moving logic from server to client, or be clear about what types of logic should be moved?" ...
77 13:20 <Emzy> I think it would be harder to optimize it. You would simply do it in sequence.
78 13:20 <emilengler> I general don't like to change existing RPC calls except new parameters are being added
79 13:20 <jnewbery> "If I'm writing a rust client or any other type of client, it would seem like the more logic there is in bitcoin-cli, the more work I have to do in rust to get access to the same functionality, and the greater chance I have of introducing bugs or inconsistencies."
80 13:21 <jnewbery> lightlike: yeah, I agree. It's nice that a small amount of code can add this functionality for many RPC methods
81 13:22 <jnewbery> emilengler: I can't see how this would be an issue. This is making the RPC method more permissive. It shouldn't impact any clients that are already using those methods
82 13:22 <PaulTroon> old code can't accidentally trigger new behavior, but new code can use it with out adding a new set of RPC calls to support
83 13:22 <jnewbery> PaulTroon: exactly
84 13:22 <emilengler> jnewbery: Are you sure? It is changing a parameter type
85 13:22 <jnewbery> the parameter type is a string in all cases
86 13:23 <jnewbery> either the hex string for the block hash or a string "@<height>" for a block height
87 13:23 <PaulTroon> changing the behavior of getblockstats would be problematic though, so it would probably have to continue to use a different system
88 13:24 <jnewbery> the only (slight) change in behaviour is for the height_or_hash parameter in getblockstats
89 13:24 <emilengler> jnewbery: Yeah but is now using RPCArg::Type::BLOCK_REF
90 13:24 <emilengler> And not RPCArg::Type::STR_HEX
91 13:24 <PaulTroon> perhaps could continue to use the deprecated form with out @
93 13:24 <emilengler> Maybe this causing issues but I'm not 100% sure
94 13:24 <jkczyz> jnewbery: Generally, I'm in favor if keeping the interface here simple given the hash at given block height can change near head. Otherwise, I would lean more towards hiding the complexity from the client.
96 13:26 <jnewbery> jkczyz: that's a good point. One response on twitter to this PR review club topic was "Ugh, I see this causing people to do a lot of stupid things that don't handle forks and orphans correctly."
97 13:26 <jnewbery> can anyone give more detail or examples of how that could cause issues?
98 13:27 <emilengler> The overloading?
99 13:27 <lightlike> getblock<hash> will stay the same during a reorg, while getblock<height> will change
100 13:27 <jnewbery> lightlike: exactly
101 13:27 <digi_james> lightlike: +1
102 13:28 <jnewbery> you could imagine someone doing `getblock<@height>` followed by `getblockstats<@height>` or similar during a re-org
103 13:28 <Emzy> this sould be expected if you are at the tip of the chain.
104 13:28 <jnewbery> and expecting the results to be for the same block
105 13:29 <Emzy> ok. that a good example, for using the hash instead.
106 13:30 <jkczyz> Also, traversing the chain should ideally follow next/prev links rather than incrementing the height
107 13:30 <jnewbery> but often, I use `get block $(getblockhash(<height>))`. That would also change if I ran it across a re-org
108 13:30 <emilengler> jnewbery: Why
109 13:31 <emilengler> It would still work
110 13:31 <jnewbery> because the inner call to getblockhash would return a different hash
111 13:32 <jnewbery> if I'm not inspecting or storing the hash returned and just using it immediately in a call, then it's basically equivalent to just calling the method with a height
112 13:32 <emilengler> jnewbery: Is getblockhash also being changed?
113 13:32 <jnewbery> yes, getblockhash() will return the hash of the block at that height in the main chain
114 13:32 <jnewbery> which can change during a re-org
115 13:32 <emilengler> Yeah I know
116 13:32 <emilengler> Oh know I got what you mean
117 13:32 <emilengler> now*
118 13:33 <emilengler> But then using heights as identifiers is general a bad approach
119 13:33 <emilengler> No matter which implementation
120 13:34 <jnewbery> right, near the tip, height shouldn't be used as a unique identifier for blocks
121 13:34 <jnewbery> Next question: What test cases have been added to these two PRs? What additional test cases could be added?
122 13:34 <jkczyz> getblock(getblockhash(<height>)) == getblock(@<height>)
123 13:35 <aj> (hmm, what if we made "@589082" give an error "height is too near the tip" ?)
124 13:35 <jnewbery> jkczyz: yeah, I think that's the test case added in 16439
125 13:35 <jkczyz> Could add tests for exceptional cases (height is out of bounds)
126 13:35 <jnewbery> a wild aj appears!
127 13:35 <jnewbery> aj: I think I like that
128 13:36 <jnewbery> jkczyz: I agree. The tests are only testing the happy case. I don't think any of the failure modes are tested
129 13:36 <jnewbery> ie giving a too-high height or a -ve height
130 13:36 <emzy> woul'd be a warning better?
131 13:37 <hanh> Could add parsing exceptional cases, something like "@xyz"
132 13:37 <jnewbery> warnings are often ignored. I tend to think of them as mostly useless
133 13:37 <jnewbery> hanh: +1
134 13:37 <emilengler> hanh: +1
135 13:38 <emilengler> Yeah good question what happens when you enter something invalid to getblock @height
136 13:38 <emilengler> Like a string
137 13:38 <emilengler> Even if its a string technically already
138 13:38 <jnewbery> emilengler: it should always be a string
139 13:39 <lightlike> aj, jnewbery: wouldn't it be necessary to be able get the block of a near-tip height, even if you know that it might change in a reorg? or do you suggest to refer to the old getblockhash(height) in this case?
140 13:39 <PaulTroon> parsing strings is always a rich source of errors
141 13:40 <emilengler> PaulTroon: +1, I think tests should be done dynamically
142 13:40 <jnewbery> 16439 actually contains two proposals for the price of one: using '@height' and parsing on the server; or using '%height' and parsing on the client. The last two commits implement the '%height' in the client approach
143 13:40 <aj> lightlike: if you want the tip, you don't know the exact height and will expect change
144 13:40 <aj> lightlike: if you want the tip, you don't know the exact height and will expect changing results -- i mean
145 13:41 <digi_james> Near the tip: If a client makes multiple (different) rpc calls using height, and would like to ensure that the return relates to the same chain, it would have to check tip/branch before, and after these rpc calls, to ensure they relate to the same chain. Actually reorgs can happen anytime, this doesn' t help.
146 13:41 <jnewbery> lightlike: I'd suggest making the error text descriptive: "height given is close to the tip. Call getblockhash(height) to get the current blockhash at heigh <x>, but be aware that it could change with a shallow reorg." or something similar
147 13:42 <aj> lightlike: i think if you want stuff near the tip you probably want to walk backwards from the tip manually, so heights are irrelevant? the times i'd want to say "tell me about block at height X" is for things well in the past where i just don't want to copy-and-paste a full hash...
148 13:42 <digi_james> * correlating calls only works with blockhash nvrmind.
149 13:43 <jnewbery> ok, final question: Which approach do you like best?
150 13:43 <emilengler> I like mine (adding a new RPC call) because I'm skeptical about overloading parameters
151 13:44 <jkczyz> Neither :) Actaully, @height in client (e.g., bitcoin-cli) I'd prefer
152 13:44 <aj> jkczyz: (that's the %height one jnewbery mentioned)
154 13:44 <emzy> The one with the rpc-client change.
155 13:45 <emzy> %height in short.
156 13:45 <fjahr> It has been said that this was requested multiple times but do we know anything more about this? Who would use this frequently but not frequently enough to use a script/alias?
157 13:45 <lightlike> I'm undecided between leave-as-is and the on by aj.
158 13:46 <PaulTroon> I like the @height change if consistently used by rest of the RPC, otherwise emilengler's is cleaner
159 13:46 <jnewbery> fjahr: good question. I think it's just convenience. People don't like those extra few keystrokes.
160 13:47 <emilengler> Also thought about this to be honest :P
161 13:47 <jnewbery> did anyone have any other questions about this PR or review in general in the last few minutes?
162 13:47 <lightlike> fjahr: also, this is one of the first things many new users will do with bitcoind. at least it was for me.
163 13:47 <PaulTroon> any sort of standards document around RPC interface?
164 13:47 <emilengler> lightlike: Yeah
165 13:48 <emilengler> PaulTroon: standards? This is a Bitcoin Core only thing
166 13:48 <hanh> fjahr my experience with this RPC is it's heavily used in blockchain extraction software.
167 13:48 <PaulTroon> emilengler: I mean convention within Bitcoin Core, like there is for coding
168 13:48 <emilengler> hanh: Like blockexplorers?
169 13:49 <emilengler> PaulTroon: I think the manpages are fine for this :)
170 13:49 <hanh> That's one example.
171 13:49 <hanh> Anywhere you need to extract blokchain data into an external storage
172 13:49 <jnewbery> PaulTroon: we've been trying to rationalize the RPC methods. Take a look at RPCHelpMan and RPCArg::Type for example
173 13:49 <jkczyz> hanh: yes, but once a block is retrieved the next/prev links could be followed
174 13:50 <jnewbery> those are new, and impose some structure on the methods and arguments
175 13:50 <PaulTroon> jnewbery: +1
176 13:50 <hanh> jkczyz Yes.
177 13:50 <emilengler> jkczyz: For this we have the prevHash field and the getblockcount
178 13:51 <jkczyz> emilengler: yeah, I think we're on the same page
179 13:52 <hanh> emilengler The problem is if you have a pipeline continuously extracting blockchain, you will keep pinging the tip of the chain. Of course, once you get the block, you can use prevHash.
180 13:52 <emilengler> Do you going from the top to the bottom
181 13:53 <emilengler> mean*
182 13:53 <jnewbery> ok, let's wrap it up there. Thanks everyone. Next week is #16115 "On bitcoind startup, write config args to debug.log (config)"
183 13:53 <emilengler> Ok, see you next week. Was funny today :)
185 13:54 <digi_james> Thanks!
187 13:54 <emzy> Thanks jnewbery and all others!
189 13:54 <jnewbery> thanks all!
190 13:54 <hanh> Thanks jnewbery
191 13:55 <lightlike> thanks!
192 13:56 <emilengler> Yes thank you all especially you jnewbery