Fix: add_new_entry converts block timestamps in local time instead of UTC - #1208
Fix: add_new_entry converts block timestamps in local time instead of UTC#1208pucedoteth wants to merge 1 commit into
Conversation
|
🤖 Automated PR review — outside automation scope, human review needed This automated reviewer verifies PoC submissions by (1) running This PR doesn't fit that shape:
The change itself (switching No action needed from the contributor — this is not a rejection, just a routing note for a human maintainer. |
|
🤖 Automated PR review — out of scope for auto-verification Thanks for the detailed writeup and the UTC timezone fix! This PR isn't something the automated reviewer can validate or auto-merge, for reasons specific to how it operates rather than any problem found in the change itself:
This looks like a solid, well-reasoned fix (converting |
get_timestamp_from_tx_hash rendered the block timestamp with datetime.fromtimestamp(unix_timestamp), which uses the timezone of the machine running the script. That timestamp decides the README entry date (%Y%m%d) and the src/test/YYYY-MM directory a new PoC lands in, so the same transaction produces different dates for different contributors. Block timestamps are UTC and the repo records hack dates in UTC: the IdolsNFT attack landed 2025-01-14 17:31 UTC and is listed as 20250114, not 20250115. The 20250918 NGP entry is off by a day for the same reason - that attack was 2025-09-17 19:02 UTC. The existing unit test already caught this but only passes at UTC+6 and east; its expected value was the author's UTC+8 rendering, labelled GMT+0000. Corrected to the real UTC value so the test is deterministic. The datetime.now() fallbacks are routed through a utc_now() helper for the same reason; it stays naive so all existing formatting is unchanged.
f5f2b09 to
ca3bb2f
Compare
|
🤖 Automated PR review — outside automated scope, needs manual review Thanks for the detailed writeup and the fix! This PR doesn't fit the pattern this automated reviewer is built to verify, so I'm leaving it open for a maintainer to review by hand rather than merging it automatically. Why:
The technical argument (UTC vs. local-time in No action needed from you — a maintainer will take it from here. |
|
🤖 Automated PR review — outside automated scope This automated reviewer verifies PoC contributions ( This PR doesn't fit that shape:
Since this reviewer's command allow-list only permits running Leaving this open for manual maintainer review rather than merging or rejecting automatically — the analysis in the PR description (UTC vs local-time timestamp handling) looks reasonable on read-through, but should be confirmed by a human before merging a change to the entry-generation tooling. |
Problem
TransactionManager.get_timestamp_from_tx_hashconverts the block timestamp with:datetime.fromtimestamp()without atzargument renders in the timezone of the machine running the script. Block timestamps are UTC seconds since epoch, so the result depends on who ranadd_new_entry.py.That timestamp is not cosmetic — it drives two things:
timestamp.strftime("%Y%m%d")→20250918 NGPtimestamp.strftime("%Y-%m")→src/test/2025-09/So the same attack tx yields a different README date, and possibly a different month directory, for a contributor in Taipei vs. Berlin vs. New York.
The repo records UTC
Checked existing entries against chain data. The discriminating case is one where UTC and UTC+8 fall on different days:
20250918 NGPis a day late — exactly the symptom of a UTC+8 machine having generated it.The existing test already catches this
test_get_timestamp_from_tx_hash_successuses block timestamp1647888693and assertsassertIn("Mar-22-2022", result), with the comment# Tue Mar 22 2022 02:51:33 GMT+0000.1647888693is actually Mon Mar 21 2022 18:51:33 UTC.Mar 22 2022 02:51:33is that instant in UTC+8 — the comment labels an author-local rendering as GMT. As a result the suite only passes from about UTC+6 eastward:Fix
datetime.fromtimestamp(unix_timestamp, tz=timezone.utc)— block timestamps rendered as UTC.Mar-21-2022 06:51:33 PM) and tightened fromassertIntoassertEqual, plus the misleading comment fixed.datetime.now()fallbacks (used when no tx hash / no timestamp is given) routed through a smallutc_now()helper, for the same reason. It returns a naive datetime, so every existingstrftime/strptimepath is unchanged.The existing
20250918 NGPentry is left alone — happy to correct it in this PR if you'd like.