Skip to content

Conversation

@roconnor-blockstream
Copy link
Contributor

@roconnor-blockstream roconnor-blockstream commented Mar 7, 2023

Tasks needed to pass CI:

  • For Mingw32, remove pthread dependency from Simplicity (upstream).
  • Merge (upcoming) libsecp256k1 release into Simplicity (upstream).

Tasks needed for public signet:

  • Add weight requirements for jets and combinantors (upstream).
  • Maximum memory allocation (upstream).
  • Other missing anti-DOS protection (upstream)?

Tasks needed for testing.

  • Ensure that the fuzzer covers libelementssimplicity.

Before merging:

  • Rebase on master.
@roconnor-blockstream roconnor-blockstream force-pushed the simplicity branch 2 times, most recently from 480f8e6 to acf8f90 Compare March 10, 2023 16:15
@roconnor-blockstream roconnor-blockstream force-pushed the simplicity branch 4 times, most recently from 0de6797 to 4d633f6 Compare March 29, 2023 18:34
@roconnor-blockstream roconnor-blockstream force-pushed the simplicity branch 3 times, most recently from 2bf20d6 to ea318a4 Compare April 18, 2023 13:45
@roconnor-blockstream roconnor-blockstream force-pushed the simplicity branch 2 times, most recently from 20d536d to 80c5d58 Compare June 14, 2023 22:31
@roconnor-blockstream roconnor-blockstream force-pushed the simplicity branch 2 times, most recently from 86223f8 to 0c562ea Compare November 14, 2023 21:24
@roconnor-blockstream roconnor-blockstream force-pushed the simplicity branch 4 times, most recently from 2485f7a to cff3732 Compare August 14, 2024 17:21
@delta1 delta1 changed the base branch from elements-22.x to elements-23.x September 2, 2024 08:17
@delta1
Copy link
Member

delta1 commented Sep 2, 2024

changed the base branch to 23.x since this was rebased @roconnor-blockstream

@apoelstra
Copy link
Member

apoelstra commented Sep 16, 2024

In 10ff078 you make simplicity a subtree (it appears not to be before then).

You should also update ci/lint/06_script.sh to add a check that the subtree has been correctly updated. (Currently passes in the commit where you add the subtree, but fails after the next commit 002c009 where you illegally add a sources.mk file to what should be a subtree directory. It appears that other cases where there is a sources.mk file, this exists upstream. For univalue it looks like they forked the repo to add this... see https://github.com/bitcoin-core/univalue-subtree ... gross.)

@roconnor-blockstream roconnor-blockstream force-pushed the simplicity branch 6 times, most recently from 6b3035d to a61faf5 Compare September 19, 2024 17:05
@roconnor-blockstream roconnor-blockstream marked this pull request as ready for review October 2, 2024 14:34
@apoelstra
Copy link
Member

CI failures look real. e.g. on the ASan build it originates in

2024-10-02T15:23:10.952000Z TestFramework (ERROR): Assertion failed 4069 Traceback (most recent call last): 4070 File "/tmp/cirrus-ci-build/ci/scratch/build/elements-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main 4071 self.run_test() 4072 File "/tmp/cirrus-ci-build/ci/scratch/build/elements-x86_64-pc-linux-gnu/test/functional/feature_taproot.py", line 1820, in run_test 4073 self.test_spenders(self.nodes[1], spenders_taproot_active(), input_counts=[1, 2, 2, 2, 2, 3]) 4074 File "/tmp/cirrus-ci-build/ci/scratch/build/elements-x86_64-pc-linux-gnu/test/functional/feature_taproot.py", line 1552, in test_spenders 4075 self.block_submit(node, [tx], msg, witness=True, accept=fail_input is None, cb_pubkey=cb_pubkey, fees=fee, sigops_weight=sigops_weight, err_msg=expected_fail_msg) 4076 File "/tmp/cirrus-ci-build/ci/scratch/build/elements-x86_64-pc-linux-gnu/test/functional/feature_taproot.py", line 1331, in block_submit 4077 assert block_response is not None and err_msg in block_response, "Missing error message '%s' from block response '%s': %s" % (err_msg, "(None)" if block_response is None else block_response, msg) 4078 AssertionError: Missing error message 'Invalid Schnorr signature hash type' from block response 'non-mandatory-script-verify-flag (Program's CMR does not match)': simplicity/empty_program,sighash/keypath_unk_hashtype_7* 4079 2024-10-02T15:23:11.007000Z TestFramework (INFO): Stopping nodes 

in feature_taproot.py

@apoelstra
Copy link
Member

code review ACK 0aeafeb

@apoelstra
Copy link
Member

4 CI failures -- the windows one might be real (test_bitcoin failing); the TSan one looks like maybe we need new tsan whitelist entries; the other two are just environment/outdated CI failures.

roconnor-blockstream and others added 14 commits October 7, 2024 14:38
Simplicity needs this fix.
git-subtree-dir: src/simplicity git-subtree-split: 86ac0f92c4b2b6d694dca85dce3b6909aed25894
also filters src/simplicity/secp256k1 from the coverage results
Take the existing function to parse script errors and extend it to parse Elements errors. SCRIPT_ERR_ERROR_COUNT is not included because it is a pseudo error.
Compare the expected script error, parsed from JSON, with the actual error that VerifyScript returns. If the JSON does not include an expected error, then skip this check.
@roconnor-blockstream roconnor-blockstream changed the base branch from elements-23.x to master October 7, 2024 18:39
Copy link
Member

@delta1 delta1 left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

utACK 5088360

@apoelstra apoelstra merged commit 2534141 into master Oct 7, 2024
@apoelstra apoelstra deleted the simplicity branch October 7, 2024 19:53
psgreco added a commit to psgreco/elements that referenced this pull request Nov 18, 2024
This reverts commit 2534141, reversing changes made to fbc01d5.
@jonathancross jonathancross mentioned this pull request Nov 25, 2024
2 tasks
delta1 added a commit to delta1/elements that referenced this pull request Dec 2, 2024
-p "src/crypto/ctaes" \
-p "src/minisketch" \
-p "src/secp256k1" \
-p "src/simplicity/secp256k1" \
Copy link
Member

Choose a reason for hiding this comment

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

In 50916cf:

Probably we didn't want to filter this. (I'm kinda surprised that the upstream libsecp is filtered. But I guess they have their own coverage analysis.)

Copy link
Member

Choose a reason for hiding this comment

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

While we're at it, we should stick /nix/store into this list and src/include/boost. I'm not sure why we need to do this and Bitcoin doesn't (well, the answer is "NixOS") but it's harmless and helpful for Russell and me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little hesitant to add nix-specific fixes here, and rather just patch the file in https://github.com/roconnor-blockstream/elements-nix/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed src/simplicity/secp256k1 from the filter as part of #1390.

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

Labels

None yet

5 participants