Removes btck_chain_get_genesis and btck_chain_get_tip.
They are trivially replaced with btck_chain_get_by_height (as indicated in the updated bitcoinkernel_wrapper.h), so I think it makes sense to trim the interface.
For btck_chain_get_tip: on master we don’t provide any guarantees that the returned block index still corresponds to the actual tip, so the extra call doesn’t seem like a regression to me.
It is equivalent to calling btck_chain_get_by_height(0).
It is equivalent to calling btck_chain_get_by_height with the
height obtained from btck_chain_get_height. In neither case do we
provide guarantees that the returned block index still corresponds
to the actual tip.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33820.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | yuvicc |
| Concept ACK | TheCharlatan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
ACK f4b3ba18d9bf3e7c2c55a98f3c3d98129a1fa6e2
This makes sense to me.
968@@ -969,7 +969,7 @@ class ChainView : public View<btck_Chain>
969
970 BlockTreeEntry Tip() const
The previous calls to tip and genesis were at least thread safe
With previous, do you mean master? I’m not sure how the current calls are not thread safe? These are read-only calls without side effects? And both in master and this PR, the caller has no guarantees that the returned tip is indeed the tip because cs_main isn’t exposed (regardless of threads)?
I wonder if we should even keep these methods now.
I agree, but I want to generalize that even more. I think Range objects should be the only public interface for their contents. Duplicated getter and count methods on the parent seem confusing and unnecessary, and are best made private imo. I’m taking that approach with py-bitcoinkernel and I think it works well. But I think given the added scope that’s better suited for a separate PR, which I’ll push out soon.
With previous, do you mean master? I’m not sure how the current calls are not thread safe? These are read-only calls without side effects? And both in master and this PR, the caller has no guarantees that the returned tip is indeed the tip because cs_main isn’t exposed (regardless of threads)?
I think there could be a rare and theoretical race where the function does not return any tip, but rather throws std::runtime_error("No entry in the chain at the provided height"); when the height is re-orged out to a lower more-work height. But this is mostly an edge case.
I hadn’t thought of the reduced-height reorg scenario, thanks for that. Reorg-safety in general is still an unsolved problem for the public interface, and this is another case of that.
Note: the problem you report already exists for objects based on the Range class whenever size() depends on chain length, such as for Chain::Entries with Chain::Entries::back() being able to throw a std::runtime_error.
Tip and Genesis methods in current master at least provided a bit better integrity. I don’t think this is important at all though. External callers should provide synchronization for this. So the two calls should just be removed imo.
Concept ACK
These were introduced before we had a dedicated iterator for traversing the chain. At this point, their benefit (thread safety) is kind of marginal.