Backend agnostic read-only DB wrapper framework #2664
Conversation
|
|
||
| proc posixPrngRand(state: var uint32): byte = | ||
| ## POSIX.1-2001 example of a rand() implementation, see manual page rand(3). | ||
| state = state * 1103515245 + 12345; |
There was a problem hiding this comment.
It is useful to have an intentionally reproducible PRNG, but this particular one isn't very good, and conveniently due to exactly this inclusion in POSIX standards has been widely studied to the point of being an example of what not to do in undergraduate CS lectures:
The output of this generator isn’t very random. If we use the sample generator listed above, then the sequence of 16 key bytes will be highly non-random. For instance, it turns out that the low bit of each successive output of rand() will alternate (e.g., 0, 1, 0, 1, 0, 1, . . . ). Do you see why? The low bit of x * 1103515245 is the same as the low bit of x, and then adding 12345 just flips the low bit. Thus the low bit alternates.
It's true though that LCGs are simple and easy, and that's useful here too. For a while Mersenne Prime was typical, but that's a bit more involved.
Xorshift might be one approach, more popular in the last few years. https://prng.di.unimi.it/ suggests it's fairly good quality as well.
https://nim-lang.org/docs/random.html uses this, even, and maybe using initRand with specific values and std/random suffices for this purpose.
| proc lastBlockNumber(e1db: Era1DbRef): BlockNumber = | ||
| var | ||
| minNum = BlockNumber(1) | ||
| maxNum = BlockNumber(4700013) # MainNet |
There was a problem hiding this comment.
This seems a bit specific? it might be enough practically to have this maxNum be efectively the max of mainnet/Holesky/Sepolia, since there presumably will never again be a network with era1 files since they'll launch merged, but then that should be noted, rather than treating the tests as only applicable to mainnnet (or if they are mainnet-specific by necessity, they probably shouldn't be framed as a general-purpose test utility).
| var | ||
| minNum = BlockNumber(1) | ||
| maxNum = BlockNumber(4700013) # MainNet | ||
| middle = (maxNum + minNum) div 2 |
There was a problem hiding this comment.
Well, (in)famously, this was one of the longstanding binary search bugs because it can overflow, even if the relevant block numbers are unlikely to overflow a BlockNumber = uint64.
Practically, it probably can't, but
| AristoDbMemory.newCoreDbRef() | ||
|
|
||
| proc newCommonInstance*(): CommonRef = | ||
| const networkId = MainNet |
There was a problem hiding this comment.
This is another case of, this is probably a bit too specific for a "backend agnostic", or it should at least be better-documented/enforced that all of this applies to mainnet only.
But there's a practical tradeoff here of, something like a Sepolia or Holesky era1 test can be done to completion much more quickly than mainnet, so constraining this to mainnet constraints tests to being only on small slices.
| # ------------------------------------------------------------------------------ | ||
|
|
||
| proc setTraceLevel* = | ||
| discard |
There was a problem hiding this comment.
None of these setFooLevel should need the discard statements, even when the when condition is false. These aren't templates, for which that would be necessary.
| mainDir: openArray[string] = mainDir; | ||
| subDir: openArray[string] = subDir; | ||
| ): Result[string,void] = | ||
| for dir in baseDir: |
There was a problem hiding this comment.
Why this particular search pattern, apparently including searching the root directory ($DirSep)?
Also there's an odd mix here between this very generic search pattern and otherwise specialized to mainnet.
| proc edgeDbMain*() = | ||
| testEra1CoreDbMain() | ||
|
|
||
| when isMainModule: |
There was a problem hiding this comment.
When imported from all_tests elsewhere, this when ensures it won't run any tests.
| key: uint64; | ||
| ): Result[Blob,EdgeDbError] = | ||
| ## Simple linear get policy. | ||
| var error = EdgeColUnsupported |
There was a problem hiding this comment.
The error symbol has a couple of other typical uses:
chroniclesloggingerrorresultsmagical variableerror
Those already often conflict, e.g., arnetheduck/nim-results#34 which triggered the workarounds in arnetheduck/nim-results#35 and while those two are somewhat baked in, less chance of random future blowups to avoid reusing the error symbol beyond that unless necessary.
| key: openArray[byte]; | ||
| ): Result[Blob,EdgeDbError] = | ||
| ## Ditto for `Blob` like key | ||
| var error = EdgeColUnsupported |
There was a problem hiding this comment.
error symbol overloading, see https://github.com/status-im/nimbus-eth1/pull/2664/files#r1779353613
| check hdrData == rlp.encode(e1Tpl.header) | ||
| check bdyData == rlp.encode(e1Tpl.body) | ||
|
|
||
| when isMainModule: |
There was a problem hiding this comment.
Is this meant to be run standalone or imported? Nothing else in this PR references this module.
|
What is this wrapper for? ie for the moment, we should not need any additional wrappers or interfaces (in fact, we have too many already) - we should not be abstracting or interfacing anything prior to there being a non-abstract version working as intended. |
Proof-of-concept, up for discussion.
Code might raise to a wish-list what is really needed, useful, etc.