Add support for chronik.block() and chronik.blocks() methods to in-node chronik-client, with associated integration testing and support script
Details
Details
- Reviewers
PiRK Fabien - Group Reviewers
Restricted Project - Commits
- rABC307ee9e448ec: [chronik-client] Add block and blocks methods to in-node chronik-client
CI builds successfully, ref test plan in D14915 to test locally
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- cc-node-block
- Lint
Lint Warnings Severity Location Code Message Warning test/functional/setup_scripts/chronik-client_block.py:38 W291 flake8 W291 - Unit
No Test Coverage - Build Status
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Failed tests logs:
====== /chronik-info: "before each" hook for "gives us the chronik info"./chronik-info "before each" hook for "gives us the chronik info" ======
Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/work/modules/chronik-client/test/integration/chronik_info.ts)
at listOnTimeout (node:internal/timers:559:17)
at processTimers (node:internal/timers:502:7)Each failure log is accessible here:
/chronik-info: "before each" hook for "gives us the chronik info"./chronik-info "before each" hook for "gives us the chronik info"
Comment Actions
remove unused import, confirm you get the connection error even on a bad block if the issue is connection
| modules/chronik-client/test/integration/block.ts | ||
|---|---|---|
| 149 ↗ | (On Diff #44378) | A parked block is a block that is valid according to consensus rules but has been rejected by avalanche. For instance if two blocks were mined at the same time, and 51% of the avalanche nodes saw block A first, block B would be parked. If your node received block B first, it could temporarily consider it to be the "good" block, and later park it when avalanche votes tell it that A is finalized. Before Avalanche and after the November 2018 upgrade, block parking was used as defense against deep reorgs. If a reorg caused a fork deeper than 10 blocks, the new chain was parked even if it had slightly more proof of worked. And a parked chain could possibly be unparked if that fork later accumulated more than twice the proof of work since the fork than the current tip. |
Comment Actions
I don't really understand the use of string for some data (sumInputSats, blocksize...), but that's not introduced by this diff.
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 93 ↗ | (On Diff #44378) | truism |
| modules/chronik-client/package.json | ||
|---|---|---|
| 3 ↗ | (On Diff #44378) | IIRC the npm package is deployed each time this version is bumped. This will cause D15216 to never be published because 0.12.0 already exists. |
| modules/chronik-client/src/ChronikClientNode.ts | ||
| 97 ↗ | (On Diff #44378) | layout |
| 103 ↗ | (On Diff #44378) | This is not at the right place, please keep ordering in par with the proto file to make it easier to review changes over time |
| 106 ↗ | (On Diff #44378) | dito |
| 108 ↗ | (On Diff #44378) | Why is this a string ? |
| 124 ↗ | (On Diff #44378) | All these fields from and including timestamp are numbers, why are they stored as string ? |
| modules/chronik-client/test/integration/block.ts | ||
| 13 ↗ | (On Diff #44378) | Why would you do such a thing when the type is already enforced at transpile time by typing the parameter ? |
| 14 ↗ | (On Diff #44378) | I don't understand what this does ? You already have types in the input and the Block_InNode members, what's the point of checking them again ? Also the name is bad imo, you're not validating a block but checking type integrity from an input. |
| 104 ↗ | (On Diff #44378) | This is not testing the block endpoint, no need to duplicate this in every test suite. Once is enough. |
| 149 ↗ | (On Diff #44378) |
The 10 block finality was not related to parking. The deep reorg protection feature was active from 2 blocks (but that's a detail, the explanation is otherwise totally correct) |
| 158 ↗ | (On Diff #44378) | dito |
| test/functional/setup_scripts/chronik-client_block.py | ||
| 2 ↗ | (On Diff #44378) | 2024 ? |
| 48 ↗ | (On Diff #44378) | Can you also repeat the last 2 steps but with invalidateblock/reconsiderblock instead of parkblock/unparkblock ? |
Comment Actions
remove ts test, remove bad server test as covered by chronik info, add invalidateblock and reconsiderblock steps
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 124 ↗ | (On Diff #44378) | I believe the issue is that chronik can only return them as string or long, and long is less convenient to deal with than string (i.e. dev can handle a string as a number easily enough, can even make it a long; handling a type like long may be inconvenient e.g. in js this requires a lib). overarching issue is that values like tokenQty must be returned as a string as they are too large for JS's number type. It would/could be nice to have safe values returned as number and unsafe values returned as string but imo just keeping everything string is acceptable. |
| modules/chronik-client/test/integration/block.ts | ||
| 14 ↗ | (On Diff #44378) | good point, we don't want to be testing typescript |
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 124 ↗ | (On Diff #44378) | Can we make sure about this rationale ? e.g. timestamp will obvioulsy fit in a number (js Date.now() returns the timestamp in milliseconds as a number), same for blockSize, numTxs, numInputs etc. Even the amount in sats should fit, this is the rationale for removing BigNumber in other diffs of yours: the supply fits in a number. |
Comment Actions
convert chronik strings to number with chronik-client for values that are below js max safe integer
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 124 ↗ | (On Diff #44378) | The chronik proto returns numbers as string for safety when numbers are 64 bits since js only manage 53 bits. Add chronik-client support to convert these values to number when we know they are below js max safe integer (i.e. the total supply of XEC in satoshis is below this value, so all satoshi params are fine. timestamps are also fine). |
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 102 ↗ | (On Diff #44430) | Why did you remove the doxygen comments ? These comments are documenting the type, you want to keep them |
| modules/chronik-client/README.md | ||
|---|---|---|
| 90 ↗ | (On Diff #44448) | did not, but will do it in this diff now as I only have this commit |