This PR updates the public kernel API documentation for validation-state output parameters that are still caller-provided:
btck_transaction_checkbtck_block_checkBoth wrappers reset the supplied validation state on entry before running validation, so callers should treat the state as overwritten in-place rather than preserving prior contents.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35189.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | yuvicc |
| Stale ACK | sedited, alexanderwiederin |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
ACK ed81aca01a058daa3d9b2d15f0db52bf38487879
I wonder what the better pattern is here. Should we always be resetting them?
1307 | - * btck_block_validation_state_create and updated 1308 | - * in-place with the validation result. 1309 | + * @param[out] validation_state Non-null, previously created with 1310 | + * btck_block_validation_state_create. Reset on 1311 | + * entry (any prior contents are overwritten) and 1312 | + * updated in-place with the validation result
Doesn't "updated in-place" imply that any value is overwritten? I think it would be better to only document the out-params that are not always updated in-place and overwritten. (or rather, try to always update them and overwrite them, where possible)
I agree that updated in-place can imply this to someone familiar with the validation-state pattern, but I think it is still worth spelling out here because this is public kernel API documentation.
From a caller’s point of view, “updated in-place” mainly says that the object passed in is mutated. It does not necessarily make it obvious that any previous result is cleared before the check runs. Since callers may reuse the same btck_BlockValidationState across calls, documenting the reset-on-entry behavior makes the contract explicit and avoids them having to know how the internal ValidationState paths behave.
Ok, but I think briefly saying Overwritten in-place with the ... is fine, if updated in-place is too ambiguous and could mean that it is not reset.
Also, what I wanted to say that this should be consistent, and it seems more important to look at the cases where it is not overwritten. Currently there is:
* [@param](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[out] block_validation_state The result of the btck_BlockHeader validation.
So this looks like a bug that should be fixed there.
good point, thanks. I updated the wording to use “Overwritten in-place...” for btck_block_check. I also fixed the inconsistent btck_chainstate_manager_process_block_header case you pointed out.
ACK ed81aca01a058daa3d9b2d15f0db52bf38487879
@w0xlt can you reply to #35189 (comment) & #35189 (review)?
Should we always be resetting them?
With the current public kernel API wrappers, yes. ValidationState is only updated on failure in many validation paths; a successful check can just return true without rewriting the state. So if a caller reuses a state object after a failed check, the old INVALID result could otherwise carry over into a later successful call.
1392 | - * btck_block_validation_state_create and updated 1393 | - * in-place with the validation result. 1394 | + * @param[out] validation_state Non-null, previously created with 1395 | + * btck_block_validation_state_create. 1396 | + * Overwritten in-place with the validation 1397 | + * result.
The btck_transaction_check docstring should use the same wording as well, no?
Done. Thanks.
Document btck_transaction_check and btck_block_check validation state output parameters as overwritten in-place. This matches their reset-on-entry behavior and avoids implying callers should preserve prior state.
re-ACK 0358c26d427a21c147c96b91e6d4f2261cb3ac46