Untested: libconsensus: Expose VerifyHeader

jtimon commented at 11:43 pm on August 9, 2016: contributor

VerifyHeader basically calls CheckBlockHeader and ContextualCheckBlockHeader for a given CBlockHeader (and all the required parameters).

Since ContextualCheckBlockHeader previously depends on CBlockIndex, some kind of C-compatible API for it must be exposed if we want a complete libconsensus to remain storage agnostic. Alternative implementations of BlockIndexInterface are more than welcomed. If there’s parts of this PR that you think would be acceptable separately, please, say so. If you come up with alternative preparations for making this PR (or an alternative) smaller, that’s welcomed too. And nits, if I haven’t said so before, are always welcomed.

We could also expose CheckBlockHeader independently. An SPV wallet using libconsensus for verifyscript already would bea easily able to also easily check CheckTransaction (which @NicolasDorier is working on) and CheckBlockHeader without any context. If we do so, we could easily move the time checks from ContextualCheckBlockHeader to CheckBlockHeader if we accept the current time of the node as a “non-contextual” parameter for CheckBlockHeader. Obviously time is context, but being pragmatic, these checks are to easy not to expose in CheckBlockHeader and everybody can be expected to have a clock they can trust (the error margins in your clock seem quite safe to me and, maybe more importante, VerifyHeader is going to use the caller’s clock anyway).

Please feel free to ask for squashes as well.

Replaces #5995

Depdendencies:

Consensus: MOVEONLY: Move functions for header verification #8337
finish implementing bitcoinconsensus_create_consensus_parameters (and remove whatever is not needed anymore from the base_blob<> preparations).

Death TODOs:

~~- Hide uint256 and DifficultyAdjustmentInterval() in Consensus::Params~~: the create interface should be enough. - [ ] The interface for CBlockIndex (const BlockIndexInterface* iBlockIndex) could be hidden in libconsensus using the same create/destroy blockindexInterfaceTrick, even if some of the parameters are going to be function pointers. Or we could offer both and see what happens…discussion welcomed here, but allowing BlockIndexInterface* to be passed as void* it’s in the todo list whether for necessary or just to see how it looks like. If we ever need a DifficultyAdjustmentInterval-like method internally, void* will be transparent. It’s simply the same argument and we should do the same. ~~- [ ] new warnings~~

jtimon force-pushed on Aug 9, 2016

jtimon renamed this:
~~Untested: libcosnensus: Expose VerifyHeader~~
Untested: libconsensus: Expose VerifyHeader
on Aug 10, 2016

jonasschnelli added the label Refactoring on Aug 11, 2016

luke-jr commented at 3:11 am on August 29, 2016: member

This API doesn’t look C-compatible. Am I missing something?

Dislike replacing uint256 with char arrays, but I’m not sure what the best alternative is; is it possible to make uint256 a C-compatible struct?

jtimon commented at 8:17 am on August 29, 2016: contributor

I guess a struct or a typedef would work.

What do you think is not C compatible? What am I missing?

luke-jr commented at 8:40 am on August 29, 2016: member

consensus/consensus.h isn’t currently exposed in libconsensus, but namespaces, the & operator, and classes won’t work in C. The function also needs to be wrapped in an extern “C” block to get the right ABI.

jtimon commented at 9:33 am on August 29, 2016: contributor

consensus/consensus.h is not to be exposed in libconsensus. I didn’t knew & wasn’t C, but that can be trivially changed to pointers. I didn’t knew namespaces weren’t C either. Consensus::Params was a big mistake then…talking it out of the namespace is going to be disruptive.

On Mon, Aug 29, 2016 at 10:41 AM, Luke-Jr notifications@github.com wrote:

consensus/consensus.h isn’t currently exposed in libconsensus, but namespaces, the & operator, and classes won’t work in C. The function also needs to be wrapped in an extern “C” block to get the right ABI.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub #8493 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AA9jSuaa_riChbz-VT9yCbliW3Kn7I9qks5qkpspgaJpZM4Jgoam .

sipa commented at 9:37 am on August 29, 2016: member

Please don’t go turn the entire codebase into C just because we want a C-capable API.

You can create a wrapper object in C that just contains a pointer to a malloced Consensus::Params. You could have a libconsensus_params_create(…) function that calls new Consensus::Params() and sets some fields in it, and return it cast to a void*. The caller would be required to call libconsensus_params_destroy() afterwards to clean it up.

jtimon commented at 9:54 am on August 29, 2016: contributor

Yes, or just recreating the struct without the namespace and convert it inside the exposed function. We can slowly replace Consensus::Params for ConsensusParams or whatever later. Still, had I known namespaces weren’t C, ConsensusParams could have lacked a namespace from the beginning and this wrapping wouldn’t be necessary.

sipa commented at 10:01 am on August 29, 2016: member

Yes it would be.

Consensus::Params is a neatly encapsulated C++ class, as it should be. It has private fields and getters, and we’re free to change its internal representation from time to time (and we have done so many times).

Converting it to a C struct with a stable representation would be an incredible burden if we’d want to start adding extra fields. It being in a namespace doesn’t matter for this at all.

Please… create a wrapper that constructs C++ objects for the C interface. Not doing so would very badly limit the possible changes we can make later.

jtimon commented at 10:46 pm on August 30, 2016: contributor

Yes, there’s no other clean way now. I still lament not having even followed my own plan of keeping Consensus::Params as C-compatible as possible…by forgetting namespaces (really, thanks @luke-jr for pointing that out).

But yeah, I’ve previously discussed with @NicolasDorier that we could wrap all fields in something like struct BlockIndexInterface https://github.com/bitcoin/bitcoin/pull/8493/commits/fb9692bc74a38233c082d90566051a5ea7eb6537 around set_pointer_function_for_pseudo_method_x() style functions as an interface and that would work similarly to not wrapping it with some small advantages even if we wanted to have the inside written in C, which I don’t realistically think about anymore in any case.

In this case, as you say, it is as easy as two functions: create_params() and destroy_params().

Thanks @luke-jr @sipa @NicolasDorier for this tremendously useful feedback.

in src/script/bitcoinconsensus.h: in 46b8482a7e outdated

 5@@ -6,6 +6,9 @@
 6 #ifndef BITCOIN_BITCOINCONSENSUS_H
 7 #define BITCOIN_BITCOINCONSENSUS_H
 8 
 9+#include "consensus/interfaces.h"
10+#include "consensus/params.h"
11+

jtimon commented at 10:54 pm on August 30, 2016:

NACK these two includes here.

jtimon force-pushed on Aug 31, 2016

jtimon commented at 10:40 pm on August 31, 2016: contributor

Rebased on top of #8337. For convenience, one can see the changes without the moveonly in: https://github.com/jtimon/bitcoin/compare/0.12.99-consensus-moveonly-header...jtimon:0.13-consensus-header

As discussed:

consensus/params.h is no longer included by script/bitcoinconsensus.h: bitcoinconsensus_create_consensus_parameters and bitcoinconsensus_destroy_consensus_parameters are created instead.
This also saves all the preparations for using a 32 byte array instead of a uint256 in Consensus::Params since its only going to be exposed in the create function: no need to disrupt any of the 3 hashes in Consensus::Params nor the convenience method. Quite a few diffs have been saved here, thanks again.

The TODOs will be updated at the end of the OP, but here’s the current summary:

finish implementing bitcoinconsensus_create_consensus_parameters (and remove whatever is not needed anymore from the base_blob<> preparations).
The interface for CBlockIndex (const BlockIndexInterface* iBlockIndex) could be hidden in libconsensus using the same create/destroy blockindexInterfaceTrick, even if some of the parameters are going to be function pointers. Or we could offer both and see what happens…discussion welcomed here, but allowing BlockIndexInterface* to be passed as void* it’s in the todo list whether for necessary or just to see how it looks like.
new warnings

jtimon force-pushed on Sep 1, 2016

jtimon force-pushed on Sep 2, 2016

jtimon force-pushed on Sep 3, 2016

jtimon commented at 5:03 pm on October 11, 2016: contributor

Added some commits fixing some nits I received on IRC. Didn’t squashed in case someone doesn’t like the changes.

jtimon force-pushed on Oct 19, 2016

jtimon commented at 10:23 pm on October 19, 2016: contributor

Squashed, should be almost-identical to previous de7a2bb but with a cleaner history.

Short term next steps:

Unify consensus/functionptr and consensus/interfaces
Create header_verify.h instead of declaring the functions in consensus/consensus.h
Pay attention to further feedback

jtimon force-pushed on Oct 19, 2016

jtimon commented at 10:46 pm on October 19, 2016: contributor

Unify consensus/functionptr and consensus/interfaces breaks previous da18576

jtimon force-pushed on Oct 20, 2016

jtimon commented at 1:59 am on October 20, 2016: contributor

More changes on top of 9628c76 :

Unify consensus/functionptr and consensus/interfaces
Create header_verify.h instead of declaring the functions in consensus/consensus.h Also, rebased and some other minors fixes.

MOVEONLY: Header verification to consensus/header_verify.o 4d1767593d

Consensus: Introduce BlockIndexInterface with CBlockIndex interfaces 4debc33094

Consensus: hack-ish: Introduce in base_blob<>: GetDataArray and explicit constructor from char* 5f3937b3f1

Consensus: Introduce CoreIndexInterface implementation for BlockIndexInterface

...that simply wraps CBlockIndex

232ca3bbac

Consensus: Artificial: Introduce wrapper for header_verify and pow:

- GetNextWorkRequired
- CalculateNextWorkRequired
- ContextualCheckBlockHeader

they depend on CBlockIndex, but libconsensus can't

1bd9c742cf

Consensus: Disruption: Decouple pow.o and header_verify.cpp from chain.h and CBlockIndex

Also Build pow.o and consensus/header_verify.cpp within libconsensus

0a1ff996c5

libconsensus: Expose bitcoinconsensus_verify_header() df8ee94b15

libconsensus: Expose C interfaces for Consensus::Params ad3ac37387

libconsensus: Expose C interfaces for BlockIndexInterface a4785f6547

jtimon force-pushed on Dec 3, 2016

jtimon commented at 7:15 am on December 3, 2016: contributor

Needed rebase after renaming main.o, see #9260 The needed rebase was clean modulo fixed nits in #8337 .

in src/consensus/cppwrappers.h: in 0a1ff996c5 outdated

17@@ -17,17 +18,17 @@ namespace Consensus { struct Params; };
18 
19 inline unsigned int GetNextWorkRequired(const CBlockIndex* pindexLast, const CBlockHeader *pblock, const Consensus::Params& consensusParams)
20 {
21-    return PowGetNextWorkRequired(pindexLast, pblock, consensusParams);
22+    return PowGetNextWorkRequired(pindexLast, CreateCoreIndexInterface(), pblock, consensusParams);

jtimon commented at 9:28 pm on December 4, 2016:

CreateCoreIndexInterface() should be only called once. Use a global instead.

jtimon commented at 6:57 am on June 19, 2017: contributor

This has been needing rebase for a while. Closing for lack of interest.

jtimon closed this on Jun 19, 2017

fanquake moved this from the "In progress" to the "Plans and discussion" column in a project

DrahtBot locked this on Sep 8, 2021

Untested: libconsensus: Expose VerifyHeader #8493