quotingFCMP++ inflation safeguards are in the open integration PRs. AI review already flagged issues. Tree building stays off on master until the FCMP++ library is treated as final. It will be gated by a compiler flag (selsta’s preference) instead of a long-lived side branch. A Carrot output spent with the old signature type can get a second key image, which is an inflation risk. An earlier block on that path was not enough: a height check can overflow. The new fix is https://github.com/seraphis-migration/monero/pull/499. Dropping the 24-hour grace period at the fork would make leftover transactions fail on purpose. jberman would rather keep it and harden the interim code. Dropped payments must be recreated. Building FCMP transactions early looked riskier.On the #11425 AI review (https://github.com/xmrack/monero-review/issues/1011), the “critical” finding was a copy-pasted database snippet that tests should catch once the code is on. The unlock-time tree-growth note is moot: timelocks are already set for removal.
nevent1q…v63l
thomasbuilds: I can’t be there for the meeting, sharing this for potential discussion:
thomasbuilds: xmrack's AI review for 11425 flags a “critical” issue. The description seems consistent with my own second review point of the PR.
thomasbuilds: What's new though, that I haven't raised is F2 (medium), "shared custom unlock_time makes one unbounded tree growth"
rucknium: Isn't that topic better for #no-wallet-left-behind meetings?
rucknium: AFAIK here's the link for that AI review: https://github.com/xmrack/monero-review/issues/1011
thomasbuilds: as you wish
jpk68: I saw that too, and it would be good to make sure people are aware of this, but that's just me
thomasbuilds: vuln is commented out right now. but plan is to uncomment it.
thomasbuilds: https://github.com/monero-project/monero/pull/11425#discussion_r4172805190
rucknium: Looks like the meeting is going to be short, so there is room for other topics. I would defer to jberman on whether it make sense to discuss it at this meeting.
jberman: Good with me to discuss, I planned to discuss this issue https://github.com/seraphis-migration/monero/pull/499 today as well
rucknium: 3. FCMP++ to-do list status. Programming tasks. Reviews and audits. FCMP++ Integration Audit Overview. Network upgrade schedule Gantt chart.
jberman: On upstream FCMP++ integration PR's: the outstanding PR's include all code that is meant to prevent inflation at consensus. I'm working through AI reviews now, which have found some notable items
sgp_: jberman: some of the tree building code from phase 3 is commented out like we talked about, is there a reason why they shouldn't be uncommented prior to getting the quotes? Or is something blocking them or whatever
jberman: Uncommenting would have the tree built on any monerod running from the master branch. The upstream FCMP++ lib code has been modified a few times over the last months in ways that render the prior tree execution incompatible with those changes
jberman: So I was thinking it may make sense to wait until we want to declare the upstream FCMP++ lib code definitively finalized such that no future changes to the tree are expected
jberman: I could also have it be off by default with a compiler flag, and then we can remove that compiler flag later to enable
jberman: (and someone can compile with it turned on)
sgp_: would an fcmp++ branch help or would it make things even more annoying?
jberman: it would be nice to have this all on master branch sooner rather than later so it's kept at pace with the (fairly rapid) changes on master
jberman: and not expect every new PR to also be applied to an fcmp++ branch
jberman: maybe selsta will have an opinion on that too
selsta: compiler flag seems best for me
sgp_: so it's just commented only because of build related things, not because it's unfinished, that is the main thing I wanted to check on
jberman: right
sgp_: gotcha, then it's probably fine from the rfq perspective
jberman: I'll do the compiler flag as well
jberman: Re: https://github.com/seraphis-migration/monero/pull/499 . The context: Carrot outputs and legacy outputs use a distinct key image generator. If a Carrot output was to be used in a CLSAG, the code would use the legacy (biased) key image generator producing a distinct key image for that Carrot output. Thus if we allowed CLSAG with Carrot outputs, and FCMP++ with Carrot outputs, then someone could double spend a Carrot output
rucknium: What if this time there was no "double fork" 24-hour grace period? It would inconvenience users at the fork height, but maybe it would be worth it to make the process safer.
rucknium: IIRC, last hard fork, "leftover" txs were dropped anyway because of an insufficient fee bug.
rucknium: "This time, it's deliberate"
jberman: The solution in https://github.com/seraphis-migration/monero/pull/446 is to prevent CLSAG rings that contain Carrot outputs. However, it relied on the code that gets the "max height" of a ring member to be correct, and unfortunately it isn't since the ring member's id calculation can overflow and wrap around, which causes the "max height" to be set to the wrong value. The solution in 499 addresses this by preventing ring members from overflowing at the fork, and also double checking that ring member heights are increasing or the same
jberman: rucknium: I'm not strongly opposed to this. It's a nice-to-have that we still need to validate is even working correctly (and txs aren't getting dropped)
jberman: We'd essentially be saying some expected valid txs might get dropped around the time of the fork, which isn't ideal
jberman: We can do everything we can to make sure this interim fork code is airtight to make sure there isn't inflation. It does widen the surface for things to go wrong though
rucknium: What would be the risks if wallet software started constructing FCMP-compatible txs 10 blocks before the fork height?
jberman: Immediate answer is I think that would a riskier change. If we were to allow such txs into the pool early, we'd have to modify a lot of consensus critical code for verification too that has to make sure not to allow those txs into the chain until later
ap4sh: (hi) if some pending txs get dropped at the fork, will wallets recover and resend them, or will users need to recreate those payments?
jberman: they would need to recreate those payments
rucknium: AFAIK, wallet software, at least the GUI and CLI, notify users if their txs fail to be confirmed after some time. Is that true?
rucknium: This will probably be 100 txs that are ejected from the txpool if no one spams during the fork.
jberman: They notify an error if the tx doesn't reach the node's pool (or is kicked from the pool)
rucknium: Last time, someone spammed right before the fork. (Someone claimed responsibility for this at a MoneroKon or Monerotopia conference years later IIRC.)
rucknium: Does someone want to discuss this ^ topic? > thomasbuilds: I can’t be there for the meeting, sharing this for potential discussion:
rucknium: > xmrack's AI review for 11425 flags a “critical” issue. The description seems consistent with my own second review point of the PR.
rucknium: > What's new though, that I haven't raised is F2 (medium), "shared custom unlock_time makes one unbounded tree growth"
rucknium: The AI review is here: https://github.com/xmrack/monero-review/issues/1011
jberman: Personally I'd prefer not to remove the grace period fork and get this code correct. But if the idea to remove it and allow some txs to get dropped at the fork gets more support, I'm not opposed
jberman: Yea, I'll discuss that as well
jberman: That "critical" issue would've been caught immediately once the code is enabled by (multiple I'm pretty sure) tests. It's a result of a snippet of DB code that I copy pasted into a different spot from our development branch because I felt it would make the code cleaner. That was a poor mistake on my part
jberman: I made some other changes on that branch from our development branch in aims of making the code cleaner and stronger based on some comments I've gotten in different places over the months
jberman: With the compiler flag for this FCMP++ integration code, we could turn it on for tests as well. I'm also going to introduce significantly more testing that we have in our development branch already, into the upstream master branch in later PR's (because it uses prover code, and I wanted to isolate consensus changes for review to focus in on the major potential consensus issues)
jberman: If we had all this stuff in 1 PR FWIW, I'd be working through 1000 LLM comments at this point
jberman: On "shared custom unlock_time makes one unbounded tree growth": this is yet another reason to drop the timelock feature. We're settled on the retroactive deprecation date so that won't be an issue
jberman: I guess I can introduce that consensus rule into this PR as well
rucknium: jberman: That's what I thought as well. It's a moot point.
https://libera.monerologs.net/monero-research-lab/20261007#c713258
nevent1q…7zwe
Nick Sparrow on Nostr: An inflation vulnerability was found in the FCMP++ code. Devs are fixing it, and ...
An inflation vulnerability was found in the FCMP++ code. Devs are fixing it, and FCMP++ stays off mainnet until the review is complete. 🫡