Force linting scripts to align rust toolchain with latest nix shell and not latest nightly

[benalleng]

Closes #1521

I opted to keep the lint scripts instead of the nix build .#checks.x86_64-linux.payjoin-workspace-clippy suggestion in the issue since these scripts catch version isolation bugs better than an `--all-features- lint

The second commit fixes a regression from #1531. I ACKed without thinking that the CI job did not acutally run an FFI CI job.

Pull Request Checklist

Please confirm the following before requesting review:

• I have disclosed my use of
  AI
  in the body of this PR.
• I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

[coveralls]

Coverage Report for CI Build 25684403347

Coverage remained the same at 85.294%

Details

• Coverage remained the same as the base build.
• Patch coverage: No coverable lines changed in this PR.
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	13668
Covered Lines:	11658
Line Coverage:	85.29%
Coverage Strength:	395.82 hits per line

---

💛 - Coveralls

[benalleng]

A bit convoluted to test but you can checkout to the latest broken lint commit and then cherry-pick the latest flake lock update to get the nightly toolchainin the dev shell that catches this.

git checkout 48cc7c760bd3476d15250f6bc5d36c1202fb8d33

then run

nix develop -c ./contrib/lint.sh

and it will pass
then to see the fix

git cherry-pick 69089be4c2ed90c7f98cf9c26194ec0b76da98bb

then you can run the test again and it will fail

nix develop -c ./contrib/lint.sh

Alternate test method

Also you can test the logic specific in this PR by reverting the lint fix commit.

git revert 24840c5812202c095ac8bbf41b9a429a32815149

[nothingmuch]

nix develop && ./contrib/lint.sh

this seems like AI hallucination, nix develop spawns a nested environment so && ./contrib/lint.sh will run after the devshell has exited if it exits with 0, and will inherit the environment of the current shell not the dev shell

if you're using direnv that might obscure things because the parent shell, if it uses direnv, will still track the updated devshell's environment variables

[nothingmuch]

concept NACK re recursive re-execution of self via conditional wrapper

i think a simpler approach is to just switch the Lint GH job from using rustup based toolchain to use nix develop -c instead of having it exec the script and have the script do that to itself

[benalleng]

this seems like AI hallucination

Only pure meat hallucination on that one. Just a bit too hasty trying to combine commands I ran individually

[nothingmuch]

Suggested change

	- name: "Use cache"
	uses: Swatinem/rust-cache@v2

i suspect this depends on having rust tooling installed, please confirm:

1. does it work without the dtolnay/rust-toolchain bootstrap? if not, is it fine to mix nightly toolchains, rustup one to manage the cache and nix one to actually build and run things?
2. does it have any effect (iirc clippy will do cargo check, does the output of that get cached? does caching via github actually save time?)

[benalleng]

The caching does seem to still work. Here is a run after the workflow changed so no cache.
https://github.com/payjoin/rust-payjoin/actions/runs/25561782784/job/75034980403?pr=1536#step:3:1 followed by a subsequent run with a cache even after the rust toolchain install had been removed https://github.com/payjoin/rust-payjoin/actions/runs/25562206670/job/75036441940?pr=1536#step:3:1

Even with using the dev shell the rust cache seems to be a substantial time saver.

[nothingmuch]

caching definitely makes sense if it's impactful, quick feedback is always helpful.

if dtolnay/rust-toolchain is still needed for Swatinem/rust-cache then please just add a big scary comment that makes it clear that the lints are running using nix provided rust toolchain and that the additional rustup provided one is there only for the cache action to work

[nothingmuch]

ugh, github's new UI is such a buggy piece of shit, but in addition to being buggy it's also confusing, i clicked into the diff and thought i was looking at a recent one and concluded that only one copy of the rust-toolchain action was removed for some reason

[DanGould]

running nix develop multiple times smells to me.

Every time nix develop runs

• Nix evaluates the flake/derivation from scratch to compute the dev shell's environment
• It checks the store for all dependencies (even if cached, this involves hash verification)
• A new shell process is spawned, environment variables are set up, shellHook runs

maxic-nix-cache only eliminates downloads.

Robot suggests the following environment setup step as best practice.

- name: Set up dev environment
    run: nix print-dev-env >> $GITHUB_ENV

but I'm not sure yet what best practice here actually is, but my hunch is that they're doing it right at fedimint. Environment setup and tool running are separate steps that should stay separate to be clean IMO. Non-blocking, but needs to be fixed after.

[DanGould]

Correction:

The shim that does this right seems to be nicknovitski/nix-develop: runs nix develop once, writes the env diff to $GITHUB_ENV / $GITHUB_PATH so subsequent steps inherit it. It lets Swatinem/rust-cache key on the nix-provided rustc. It doesn't work if we depend on shellHooks for setup, but we don't.

Still non-blocking. Leaving as concrete pointer for the follow-up (please make an issue when u merge/close)

[DanGould]

fedimint does a bunch of complicated stuff which is great for them but loses the multi-toolchain running. They have unique derivations for Ci jobs rather than defining them primarily in YML. And have their own runners that stay warm with their caches. Different thing we don't need yet.

[nothingmuch]

microsoft donating 1s more of cpu time seems like a fair price to pay for not introducing a github action dependency that isn't really necessary, or rolling our own and debugging string escaping rules in bash in javascript in yaml

nix develop -c is reliable and predictable, and also clarifies to the reader under what environment something is being run on github

if we reach the point where it's more than just a sprinkling here or there and that action is legitimately useful i'd argue things are too tightly coupled to the nightmare that is github actions

[benalleng]

Interesting what fedimint does here just running their whole pre-commit hook https://github.com/fedimint/fedimint/blob/c1f97273aa59c1580f03a38530ad104072cdb412/.github/workflows/ci-nix.yml#L62

[DanGould]

Same problem as the rust.yml here. Doesn't make sense to set up the environment with every command. Set up the environment once, then run your commands. very related to #1536 (comment). For follow up.

[benalleng]

what is the primary impetus for keeping the ffi-lint and (payjoin + cli) crate lints separate?

[nothingmuch]

hmm, why does this snippet even live in CONTRIBUTING.md? since that's intended to run on a user's local machine and help preempt CI failures, IMO it would be better, in a followup PR:

1. move it to a separate shell script in the root contrib directory
2. provide a flake app (i.e. derivation suitable for use as nix run .#lint-commit-hook, which would be thin wrapper around the shell script that runs the script in a consistent environment, and also use it in a flake check for sandobxed execution
3. in docs assume either nix develop is opted in via direnv or manually or that that rustup is deliberately being used instead, i.e. it's the developer's business any example commands should not interfere with that choice, much like the test scripts do

this way if the reccomendation changes people don't have to update their git hook

[benalleng]

I am going to drop this for a followup for now