Skip to content

fix(tally): fix shared default ciphertext in tally selection#810

Merged
JDziurlaj merged 6 commits intoElection-Tech-Initiative:mainfrom
mdevolde:fix/tally-default-factory
Apr 13, 2026
Merged

fix(tally): fix shared default ciphertext in tally selection#810
JDziurlaj merged 6 commits intoElection-Tech-Initiative:mainfrom
mdevolde:fix/tally-default-factory

Conversation

@mdevolde
Copy link
Copy Markdown
Contributor

Issue

Fixes #809

Description

This change updates CiphertextTallySelection.ciphertext to use default_factory instead of a shared ElGamalCiphertext instance as a dataclass field default. That ensures each CiphertextTallySelection gets its own ciphertext value rather than sharing the same default object across instances.

The behavior avoids unintended shared state if the default ciphertext is ever mutated in place, and also resolves the dataclass validation error raised by Python 3.11 for mutable defaults.

I ran the tests locally: poetry run pytest tests/unit, poetry run pytest tests/property, poetry run pytest tests/integration (on python 3.9.21, on windows 11). I built the project with poetry build on the same environment.

Testing

Run the relevant tally-related test suites with:

poetry run pytest tests/unit/electionguard
poetry run pytest tests/property/test_tally.py

These validate that CiphertextTallySelection still behaves correctly.

@mdevolde
Copy link
Copy Markdown
Contributor Author

@lprichar @jungshadow hey, sorry for tagging you, I just wanted to know if you're still maintaining this repository?

@jungshadow
Copy link
Copy Markdown

Hi @mdevolde! Thanks for the PR and the flag, and sorry I didn't see this earlier. You probably followed that ElectionGuard moved from Microsoft to the Election Technology Initiative, and we're currently trying to determine where our resources should go. The .NET implementation recently received an update and is currently being used by US election tech providers. My heart is always with Python, and I'd personally like to maintain this version, too, but we need both a business need and funding for it. Give me a few days to check with @JDziurlaj on what it would take to set up PR validation here, and I'll see if we can at least merge this in. Thanks again!

@mdevolde
Copy link
Copy Markdown
Contributor Author

mdevolde commented Apr 3, 2026

Hi @jungshadow!
Thank you very much for your response and your honesty! I completely understand your point of view.
Even if only a minimal/occasional maintenance routine can be put in place for this repository, it would likely be beneficial.
It would be a loss if this repository could no longer benefit from community patches and became unusable.

@jungshadow
Copy link
Copy Markdown

@mdevolde, I hear you, and I agree. We had high hopes that this project and its goals would attract significant financial support. Sadly, it hasn't worked out that way, so we're trying to be as strategic as possible with the support we have.

@JDziurlaj JDziurlaj closed this Apr 8, 2026
@JDziurlaj JDziurlaj reopened this Apr 8, 2026
@JDziurlaj JDziurlaj closed this Apr 8, 2026
@JDziurlaj JDziurlaj reopened this Apr 8, 2026
@JDziurlaj JDziurlaj closed this Apr 8, 2026
@JDziurlaj JDziurlaj reopened this Apr 8, 2026
@JDziurlaj
Copy link
Copy Markdown
Contributor

I apologize for the noise on this PR, I have to close and reopen the PR to force GitHub actions.

@mdevolde
Copy link
Copy Markdown
Contributor Author

mdevolde commented Apr 8, 2026

I apologize for the noise on this PR, I have to close and reopen the PR to force GitHub actions.

No problem !
If you add this in an action workflow, you can trigger it manually:

on:
  workflow_dispatch:

@JDziurlaj JDziurlaj self-requested a review April 10, 2026 12:25
@JDziurlaj JDziurlaj force-pushed the fix/tally-default-factory branch from 6a3b25d to 0042052 Compare April 10, 2026 12:26
Copy link
Copy Markdown
Contributor

@JDziurlaj JDziurlaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@JDziurlaj
Copy link
Copy Markdown
Contributor

Once we get a signed CLA, we can merge this change, @mdevolde. Please view my GitHub profile and send me an email, and I'll respond with a Docusign. Thanks!

@JDziurlaj JDziurlaj merged commit 7020e61 into Election-Tech-Initiative:main Apr 13, 2026
4 checks passed
@mdevolde mdevolde deleted the fix/tally-default-factory branch April 13, 2026 12:47
@mdevolde
Copy link
Copy Markdown
Contributor Author

Thank you @JDziurlaj !
It is possible that I reopen some issues/PRs regarding the usability of this lib on the latest versions of python, if I encounter any further issues.
I really appreciate that you’ve taken the trouble to maintain this repository, even if it’s only on an occasional schedule.
Are you planning to bump the package version and upload a new version to PyPI?

@mdevolde
Copy link
Copy Markdown
Contributor Author

Update: before bumping lib version and pushing on PyPI, I made a little other PR #813, to ensure that all the dependencies are compatible with our usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐞 CiphertextTallySelection uses a shared default ElGamalCiphertext instance

3 participants