Note that we have a minor difference in behavior vs Fulcrum:
Fulcrum has an additional check that height, cp_height and start_height are < Storage::MAX_HEADERS (100'000'000), and returns a "Invalid height/cp_height" error message in such a case. We don't return such an error before these values hit max_int32 + 1.
I don't see any value in hardcoding such an additional limit, as these parameters are all bound by more restrictive values (tip_height, max_count=2016) before we even do any computation. So we still get an error in the same situation, but a different one.
Details
Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Commits
- rABC98839217de42: [chronik] implement electrum RPC blockchain.block.headers
ninja check-functional
Check that the results and error messages are the same as the ones returned by Fulcrum (with the above mentionned minor difference)
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- blockchain.block.headers
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Event Timeline
| chronik/chronik-http/src/electrum.rs | ||
|---|---|---|
| 337 ↗ | (On Diff #52079) | last time I checked positive i32 was very close to u32 :) You should use that in your Result and maybe call it json_to_u31 or something like that |
| chronik/chronik-http/src/electrum.rs | ||
|---|---|---|
| 420 ↗ | (On Diff #52083) | also hardcoded in fulcrum: https://github.com/cculianu/Fulcrum/blob/master/src/Servers.cpp#L1287 |
| 337 ↗ | (On Diff #52079) | function renamed so i can be used for other params than just the block height (count needs the same checks) |
| 367 ↗ | (On Diff #52079) | This is the error message Fulcrum returns, the previous error message "Invalid height"" was incorrect |
Comment Actions
rename helper function json_to_u31. We still return a i32, because that's what all functions expect a block height to be in the node and the indexer, but with the added guarantee that it is >=0 . Returning a u32 would just shift the problem from checking >= 0 to checking <=2**31 -1, while requiring a lot of as i32 conversions when calling the indexer functions.
Comment Actions
More questions than requesting changes
| chronik/chronik-http/src/electrum.rs | ||
|---|---|---|
| 421 | This is not a good implementation (it overrides the user request silently oO), but afaict this is also what fulcrum does... And at least it's documented | |
| 461 | Why don't you get them all by range, then serialize in a loop ? That will be more efficient | |
| test/functional/chronik_electrum_blockchain.py | ||
| 261 | ||
Comment Actions
use headers_by_range which simplifies the code a lot
| chronik/chronik-http/src/electrum.rs | ||
|---|---|---|
| 421 | Yes it is a weird behavior. It works because the actual count is part of the response, but that is a bit of a waste of bandwidth (and the hardcoded max count is even more wasteful and useless in the response) | |
| chronik/chronik-http/src/electrum.rs | ||
|---|---|---|
| 457–472 ↗ | (On Diff #52108) | headers_hex doesn't need to be mutable |