Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleaner separation of Verifier / Constraint Cheker duties #188

Merged
merged 38 commits into from
Mar 20, 2024

Conversation

JoshOrndorff
Copy link
Contributor

@JoshOrndorff JoshOrndorff commented Mar 1, 2024

The primary change this PR makes is to remove the ConstraintChecker's access to the information about verifiers.

We added this feature thinking it was necessary for use cases such as sudo-style governance, royalties, and an on-chain treasury. It turns out, this was not the case and was more a reflection of our newness to thinking in the UTXO model. I've now demonstrated royalties in #185, sudo in #186, and sketched an idea for a treasury.

This change also simplifies the aggregation macro considerably which is always welcome, especially as I prepare to experiment with strong typing in the UTXO set a la #178.

A secondary change is that it cleans up the relationship between constraint checkers and inherents somewhat and removes the last hack from the existing inherent pieces. Inherents are still messier than I want but this is largely because Substrate's notion of inserting required info into block bodies via transactions is a little messy.

@JoshOrndorff JoshOrndorff requested a review from coax1d March 1, 2024 18:38
@JoshOrndorff
Copy link
Contributor Author

Treasury Design

Here is the sketch of a council-style treasury piece I mentioned in the pr description.

treasury-dao-lifecycle

@muraca
Copy link
Collaborator

muraca commented Mar 15, 2024

I was trying to figure out why it does not crash when new_unspendable is not implemented.

I tried to add a panic at the beginning of the Timestamp create_inherent function, but nothing happens.
Then I tried to add it at the beginning of this function

fn create_inherents<V: Verifier>(
authoring_inherent_data: &InherentData,
previous_inherents: Vec<(Transaction<V, Self>, H256)>,
) -> Vec<Transaction<V, Self>> {
if previous_inherents.len() > 1 {
panic!("Authoring a leaf inherent constraint checker, but multiple previous inherents were supplied.")
}
let (previous_inherent, hash) = previous_inherents
.first()
.cloned()
.expect("Previous inherent exists.");
let current_inherent = wrap_transaction(<C as InherentHooks>::create_inherent(
authoring_inherent_data,
(unwrap_transaction(previous_inherent), hash),
));
vec![current_inherent]
}
and nothing happens as well.

@JoshOrndorff
Copy link
Contributor Author

JoshOrndorff commented Mar 17, 2024

I solved the why-doesn't-it-panic mystery in cc0758d. I had forgotten to explicitly mark the inherents with the new InherentAdapter type.

@muraca muraca self-requested a review March 18, 2024 17:37
@JoshOrndorff
Copy link
Contributor Author

JoshOrndorff commented Mar 18, 2024

One problem here is that the parachain node no longer works because the parachain inherent UTXO is now unspendable and can't be consumed by the next block's inherent. Some options:

  1. Merge it as is with parachain node broken, and implement evictions next to fix it.
  2. Change the parachain piece so it doesn't have to consume previous inherent, more like timestamp.
  3. Switch the parachain piece back to abusing UpForGrabs for now.

I'm leaning toward option # 1. I believe I have evictions working in #196 but I it seems the full CI doesn't run there. Maybe because it isn't a PR against the base branch?

@JoshOrndorff JoshOrndorff mentioned this pull request Mar 20, 2024
@JoshOrndorff JoshOrndorff merged commit 9a18761 into main Mar 20, 2024
6 checks passed
@JoshOrndorff JoshOrndorff deleted the joshy-constraint-checker-beef-up branch March 20, 2024 18:07
Copy link

Coverage after merging joshy-constraint-checker-beef-up into main will be

61.44%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
tuxedo-core/aggregator/src
   lib.rs99.14%100%100%99.10%201, 22, 84
tuxedo-core/no_bound/src
   clone_no_bound.rs36.63%100%30%37.36%24, 32–39, 56, 59–72, 74–87, 89, 91–95, 98–99
   debug_no_bound.rs32.73%100%30%33%101–105, 108–109, 24, 32–42, 58, 60–78, 80–95, 97–99
   default_no_bound.rs19.11%100%16.67%19.31%100–122, 124–131, 133, 136–147, 151–157, 32–39, 51, 54–80, 83–88, 91–99
   lib.rs100%100%100%100%
tuxedo-core/src
   constraint_checker.rs47.66%100%47.83%47.62%116–123, 129–134, 136–145, 147, 194–198, 201–206, 209–210
   dynamic_typing.rs83.33%100%70.59%88.37%
   executive.rs92.21%100%93.60%92.05%119, 145, 185, 217, 246, 257, 308, 335, 386–391, 393, 403–404, 409–410, 412, 415–416, 419, 421–422, 425–426, 428, 432–453, 455, 459–460, 462–463, 465, 468–485, 55
   genesis.rs43.69%100%42.86%43.75%102–103, 106, 158–160, 169, 36–49, 60, 62–65, 67–68, 70–72, 75–79, 81–83, 85–96, 98–99
   inherents.rs17.93%100%25%16.53%122–124, 151–158, 165–172, 178–196, 198–228, 56–58, 65–70, 72–80, 83, 85
   types.rs62.44%100%46%68.03%180–185, 82–86, 88–91, 93–99
   utxo_set.rs90.91%100%100%88.89%39–40
   verifier.rs42.86%100%32.50%48.61%106–108, 43–44, 46, 48–49, 54, 71, 78–80, 82–84
tuxedo-core/src/verifier
   htlc.rs77.96%100%44.93%85.71%107, 109, 115, 40, 42, 55, 57, 74–78, 91
   multi_signature.rs87.50%100%68.29%91.16%100–105, 45, 71
   simple_signature.rs76.87%100%58.33%82.88%65, 67, 78–82
tuxedo-parachain-core/register_validate_block/src
   lib.rs0%100%0%0%100–119, 12, 120–125, 128, 13, 130–131, 14–16, 18, 29–35, 38–46, 50, 54–56, 59–61, 64–66, 84–99
tuxedo-parachain-core/src
   collation_api.rs0%100%0%0%19–40
   lib.rs48.65%100%53.85%45.83%85–90, 94–96
   relay_state_snapshot.rs0%100%0%0%112, 128–140, 147–155, 157, 173–188, 193–202, 204–211, 223, 225, 227–233, 235–240, 242, 245–250, 252–257, 259–271, 274–286, 292–298, 303–310, 316–323, 330–337, 346–354, 362–370, 378–383, 389–394, 41, 54, 82
   tests.rs100%100%100%100%
tuxedo-template-runtime/src
   genesis.rs84.81%100%88.89%84.56%23–45
   lib.rs5.87%100%14.63%3.67%100–104, 130, 135–137, 141–143, 205, 207, 209, 211, 213, 215, 217, 219, 222, 224, 227, 233, 242–249, 253, 262–283, 286–313, 316–435, 63–68, 99
wallet/src
   amoeba.rs0%100%0%0%100–106, 109–110, 112–118, 120–127, 19–48, 51–52, 54–99
   cli.rs0%100%0%0%100, 115, 119, 125, 128, 133, 144, 150, 17, 20, 22, 31, 36, 41, 48, 65, 77
   keystore.rs0%100%0%0%30–33, 38–45, 47–48, 51, 53–59, 65–73, 76–78, 80–82, 85–91, 93–94
   main.rs0%100%0%0%100–193, 196–203, 206–209, 211–213, 216–218, 220, 222, 225–231, 234–247, 250–253, 255–266, 268, 27–99
   money.rs0%100%0%0%100, 106–113, 115, 119–124, 128, 131, 133, 136–139, 141–142, 146, 150–153, 155, 160–166, 168–172, 175–176, 180–185, 187–188, 191–202, 204, 206, 22–23, 25–40, 43, 47–60, 63–69, 72–90, 94–99
   output_filter.rs100%100%100%100%
   rpc.rs0%100%0%0%16–21, 24–26, 28–44, 47–53, 55–58, 60–61
   sync.rs0%100%0%0%103–106, 113–114, 116, 119–122, 127–129, 132–133, 136–141, 143–145, 148, 150–151, 156–159, 162–163, 170–174, 176–182, 185–187, 189–190, 193–194, 199–202, 205, 207–208, 213–216, 219, 221–222, 225–231, 233–234, 237–238, 241–242, 245–246, 250–256, 259–263, 266, 268, 271, 273, 277, 279–280, 283–284, 287–294, 296–297, 300–301, 303, 305–306, 310–312, 314–315, 317–318, 320–321, 324–326, 328–329, 331–332, 334–335, 339, 341–342, 346, 348–353, 356–357, 360–362, 365, 368–371, 373, 376–379, 382, 385–386, 389–390, 395–400, 402, 404, 409–412, 415–416, 419–424, 426, 429–430, 434–435, 437, 439–441, 443–446, 449–450, 46–50, 54, 57–58, 61, 63–77, 82–86, 88–89, 94–99
   timestamp.rs0%100%0%0%12–17, 20–27
wardrobe/amoeba/src
   lib.rs59.33%100%26.83%71.56%121, 139, 178, 81
   tests.rs100%100%100%100%
wardrobe/kitties/src
   lib.rs47.76%100%26.72%57.14%100, 102–103, 107–109, 117–119, 121–123, 127–129, 133–134, 137–139, 143–145, 150–152, 154–155, 159–161, 172–192,

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.

2 participants