search

hebasto / bitcoin

Bitcoin Core integration/staging tree
https://bitcoincore.org/en/download
MIT License
19 stars 4 forks source link

Rename `core_interface` and `core_base_interface` #241

Open fanquake opened 1 week ago

fanquake commented 1 week ago

It seems that since these were introduced, what they contain has changed / is still changing, and it's not really clear what the distinction between these two interfaces is. It'd be good if what is meant to exist in each was clarified, and then we could give then meaningful names (and it'd be clear where new flags/things should go).

fanquake commented 2 days ago

Can anyone clarify here?

hebasto commented 7 hours ago

It'd be good if what is meant to exist in each was clarified...

core_base_interface:

core_interface:

... and then we could give then meaningful names (and it'd be clear where new flags/things should go).

I'm open for proposals.

hebasto commented 7 hours ago

cc @theuni @TheCharlatan @real-or-random @ryanofsky

fanquake commented 7 hours ago

So the split seems to be based mostly on subtree vs not. I think splitting our codebase that way, is making less and less sense, given that our subtrees are basically becoming "our" code (i.e leveldb). In any case, it seems like there is atleast two issues going forward this way,:

If hardening_interface moves into core_base_interface, then core_interface is really just warn_interface, so core_interface can probably just go away entirely, and we just apply warning flags where we like?

Where does libsecp256k1 (C) fit in? What options does it get? I'm assuming just core_base_interface?

No mention of ObjC++ (for Qt), but I assume that is curently included in core_interface?

hebasto commented 6 hours ago

Where does libsecp256k1 (C) fit in? What options does it get? I'm assuming just core_base_interface?

libsecp256k1 uses only sanitize_interface. This behavior follows the master branch.

No mention of ObjC++ (for Qt), but I assume that is curently included in core_interface?

Correct. Objective C++ sources are included in the bitcoinqt build target, which uses core_interface.

hebasto commented 6 hours ago

Hardening options should be getting applied to all code (C,C++,subtree or not, and this is needed for future changes).

Currently, the hardening options are applied to all C++ code:

I admit that this looks suboptimal and can be improved.

Not applying the hardening options to C code in libsecp256k1 follows the master branch behavior. It can be reconsidered after migration to CMake.

If hardening_interface moves into core_base_interface, then core_interface is really just warn_interface, so core_interface can probably just go away entirely, and we just apply warning flags where we like?

The need to apply warn_interface to almost every build target will make the code more wordy, won't it?

fanquake commented 6 hours ago

Currently, the hardening options are applied to all C++ code: via hardening_interface in subtrees

How is that happening if subtree code, (according to your comment above), only gets core_base_interface, which does not include hardening_interface?

The need to apply warn_interface to almost every build target will make the code more wordy, won't it?

How is that any different to applying (a wrapper only) core_interface everywhere?

hebasto commented 6 hours ago

Currently, the hardening options are applied to all C++ code: via hardening_interface in subtrees

How is that happening if subtree code, (according to your comment above), only get core_base_interface, which does not include hardening_interface?

I cannot see "subtree code only get core_base_interface" in my comment. It uses https://github.com/hebasto/bitcoin/blob/fec5dcdcef59290d4c9202a6e3a56b68efb89ef7/cmake/crc32c.cmake#L88-L91

fanquake commented 6 hours ago

That makes me understand that point of these interfaces even less then. Given that the only difference seems to be warn_interface, and that not being applied to subtrees currently, would seem to be a bug (at least for leveldb).

hebasto commented 6 hours ago

The need to apply warn_interface to almost every build target will make the code more wordy, won't it?

How is that any different to applying (a wrapper only) core_interface everywhere?

It differs in a way that it adds one more line in every location:

target_link_libraries(bitcoin_some_target INTERFACE 
  core_interface 
  warn_interface
)

instead of the current

target_link_libraries(bitcoin_some_target INTERFACE 
  core_interface 
)

Besides being more verbose, it is also is more error-prone.

fanquake commented 6 hours ago

It differs in a way that it adds one more line in every location:

It doesn't. As I said in my comment about, core_interface would no-longer exist. So the diff would just be changing core_interface to warn_interface.

hebasto commented 6 hours ago

It differs in a way that it adds one more line in every location:

It doesn't. As I said in my comment about, core_interface would no-longer exist. So the diff would just be changing core_interface to warn_interface.

How would other staff (platform-specific, hardening etc) be applied to targets after "changing core_interface to warn_interface"?

fanquake commented 6 hours ago

How would other staff (platform-specific, hardening etc) be applied to targets after "changing core_interface to warn_interface"?

With core_base_interface ?

hebasto commented 6 hours ago

How would other staff (platform-specific, hardening etc) be applied to targets after "changing core_interface to warn_interface"?

With core_base_interface ?

Right. So two interfaces will be needed for targets instead of one?

hebasto commented 5 hours ago

That makes me understand that point of these interfaces even less then. Given that the only difference seems to be warn_interface, and that not being applied to subtrees currently, would seem to be a bug (at least for leveldb).

What is the point of enabling warnings in leveledb if the problematic ones are eventually disabled like that: https://github.com/hebasto/bitcoin/blob/fec5dcdcef59290d4c9202a6e3a56b68efb89ef7/src/Makefile.leveldb.include#L45 ?

fanquake commented 5 hours ago

Yes, but core_base_interface w/should be getting applied globally in some way? Is there a higher level concern with having more than one interface for a target?

fanquake commented 5 hours ago

What is the point of enabling warnings in leveledb if the problematic ones are eventually disabled like that:

Aside from all of the other warnings that aren't disabled? Fixing the two cases there is quite straight forward, and likely should just be done. I think it might also just happen for us the next time we pull the subtree.

hebasto commented 5 hours ago

What is the point of enabling warnings in leveledb if the problematic ones are eventually disabled like that:

Aside from all of the other warnings that aren't disabled? Fixing the two cases there is quite straight forward, and likely should just be done. I think it might also just happen for us the next time we pull the subtree.

Okay then. I'll submit a PR to fix it.

hebasto commented 5 hours ago

Yes, but core_base_interface w/should be getting applied globally in some way?

The point of using interfaces is to avoid global MAKEFILE-ish states, which are fragile and error-prone.

Is there a higher level concern with having more than one interface for a target?

None. Verbosity only.

hebasto commented 5 hours ago

That makes me understand that point of these interfaces even less then. Given that the only difference seems to be warn_interface, and that not being applied to subtrees currently, would seem to be a bug (at least for leveldb).

Fixed in https://github.com/hebasto/bitcoin/pull/255.

fanquake commented 4 hours ago

I think this issue can be closed after #255. Which has been updated to essentially do what I mentioned in https://github.com/hebasto/bitcoin/issues/241#issuecomment-2205419599.