Repository navigation
attest: Simpler NO_SIGN, optionally derive EPOCH - #54
Conversation
jurraca
left a comment
There was a problem hiding this comment.
ACK on the SIGNER/NO_SIGN change (103a6e5) but the ENCODED_* default paths change have tradeoffs that imo are problematic:
- the point of attestation is not to rely on the files here, but to provide an attestation that the user generated themselves, so that we can compare the attestations to the files in the tree. If the defaults are used, the user is attesting to what's already there. We want the user to provide their own encoded files, showing that they got a final result, and encoded it, and attested to those files.
- Ideally the user could provide the
ASMAP_TXTas currently, and call out to thebitcoin/contrib/asmap/asmap-tool.pyto encode the files on the fly, and generate the attestation from there. This is maybe a better approach, though it complicates the script a bit. - On attesting to a new run, the default file paths do not exist, so they are required unless the files already exist.
$ env NO_SIGN=1 ASMAP_TXT=../kartograf/out/1780588800/final_result.txt EPOCH=1780588800 ./asmap-attest
ERR: The specified ENCODED_FILLED does not exist or is not a regular file:
'2026/1780588800_asmap.dat'
103a6e5 to
907d3c7
Compare
907d3c7 to
4bccb03
Compare
|
Reworked the PR quite a bit now instead of deriving encoded file paths from the EPOCH, we derive the EPOCH from the encoded file paths if possible. Thank you for the feedback in #54 (review)! Sorry it took a while. |
|
(Rebased as well since the commits have been reworked so much). |
fjahr
left a comment
There was a problem hiding this comment.
Looks like a good improvement overall but I am still unsure if I like the interaction between the explicit EPOCH and the automated derivation. I thing it would seem most natural to me that we either let EPOCH override the automated derivation or drop EPOCH altogether and only derive from the file name. But happy to be convinced otherwise.
|
|
||
| EPOCH_filled=${filled_basename%_asmap.dat} | ||
| EPOCH_unfilled=${unfilled_basename%_asmap_unfilled.dat} | ||
| if [ "$EPOCH_filled" = "$EPOCH_unfilled" ] && [[ "$EPOCH_filled" =~ ^[0-9]+$ ]]; then |
There was a problem hiding this comment.
If I understand this correctly, this whole block just passes silently when the filled and unfilled epochs don't match and EPOCH is set. That seems unintended, if the epochs between the files mismatch we may want to fail in any case, so I think I would just move that check up as a guard clause.
There was a problem hiding this comment.
Not sure we should require that users encode asmaps into filenames matching specific patterns.
Re-organized the code and added an error if we are able to derive epochs from both encoded filenames and they mismatch, regardless of EPOCH being set or not.
Enables extracting the NO_SIGN case above the SIGNER case in the next commit.
In this case we just output the SHA256SUMS to the console.
Also guard against mismatches.
Also drop empty newline after help text since at least in bash it appears we get an additional empty one anyway.
4bccb03 to
132a5c2
Compare
hodlinator
left a comment
There was a problem hiding this comment.
I thing it would seem most natural to me that we either let
EPOCHoverride the automated derivation or dropEPOCHaltogether and only derive from the file name. But happy to be convinced otherwise.
Not sure we should enforce users encode to specific filenames. They could use the ones already in the repo iff they match hashes of the files they previously encoding locally. As of the latest push, if unable to derive an epoch from any of the filenames, we require EPOCH. (Previous push required both to be derivable or treated it as none being derivable).
If we can derive, we sanity check against EPOCH (if it is set), so users don't accidentally use the wrong files or specify a mismatching EPOCH.
|
|
||
| EPOCH_filled=${filled_basename%_asmap.dat} | ||
| EPOCH_unfilled=${unfilled_basename%_asmap_unfilled.dat} | ||
| if [ "$EPOCH_filled" = "$EPOCH_unfilled" ] && [[ "$EPOCH_filled" =~ ^[0-9]+$ ]]; then |
There was a problem hiding this comment.
Not sure we should require that users encode asmaps into filenames matching specific patterns.
Re-organized the code and added an error if we are able to derive epochs from both encoded filenames and they mismatch, regardless of EPOCH being set or not.
If this was a general tool that can be used in other contexts, I would say it definitely shouldn't. But I see this as a tool only for this particular repository and we have lots of assumptions about that baked in otherwise, like the attestation folder structure for example. We don't state this as a hard rule in the readme that the file names have to have this structure but it's in the examples etc. and we are consistent about this in the repo and if we weren't at least some of our CI would break. So I think being explicit about the file names would just be consistent with what we are doing otherwise in the repo. But it's not that relevant, since we already enforce it in other place being strict here doesn't make much of a difference. |
jurraca
left a comment
There was a problem hiding this comment.
tACK 132a5c2
Deriving the EPOCH from the ENCODED_* is good, and having to match on a specific file name syntax is acceptable for now. Regarding priority, I lean towards ENCODED_* files timestamps being checks on EPOCH, not vice versa. The narrow path being: encode it with the name and timestamp, and the script picks up the epoch from the name. Tested and the error messages and indications are appropriate.
Thanks @hodlinator
Improves ergonomics of the asmap-attest script:
SIGNERifNO_SIGNis set and only output SHA256SUMS in that case.EPOCHenv var when it can be computed off the encoded files.Inspired by discussion at #50 (comment)