Chia-Network / clvm_tools

Tools for clvm development
Apache License 2.0
51 stars 32 forks source link

Feel strange on comparing `run_program` implementation of stage_0, stage_1, stage_2 #69

Closed ChiaMineJP closed 3 years ago

ChiaMineJP commented 3 years ago

stage_0

def run_program(
    program,
    args,
    operator_lookup=OPERATOR_LOOKUP,    # <== operator_lookup can be changed by parameter
    max_cost=None,
    pre_eval_f=None,
    strict=False,
):
    ...

stage_1

class RunProgram:
    def __init__(self):
        operator_lookup = OperatorDict(OPERATOR_LOOKUP)
        operator_lookup.update((k.encode("utf8"), v) for (k, v) in BINDINGS.items())
        self.operator_lookup = operator_lookup

    # __call__  does not accept `operator_lookup` here. So `operator_lookup` can't be changed outside by parameter.
    def __call__(self, program, args, max_cost=None, pre_eval_f=None, strict=False):
        return run_program_0(
            program,
            args,
            operator_lookup=self.operator_lookup,
            max_cost=max_cost,
            pre_eval_f=pre_eval_f,
            strict=strict,
        )

stage_2

    # As well as stage_0, operator_lookup can be changed by parameter, but this is different from stage_1.
    def run_program(
        program, args, operator_lookup=operator_lookup, max_cost=None, pre_eval_f=None, strict=False
    ):
        return run_program_0(
            program,
            args,
            operator_lookup=operator_lookup,
            max_cost=max_cost,
            pre_eval_f=pre_eval_f,
            strict=strict
        )

The strangeness comes from the facts that:
stage_0: run_program accepts operator_lookup by parameter. It can be modified from outside by parameter. stage_1: run_program DOES NOT accepts operator_lookup by parameter. It cannot be modified from outside by parameter. stage_2: Does not inherit stage_1 spec. Equipping stage_0 style argument.

I feel inconsistency of run_program specification between stage_1 and stage_2. It is clean if stage_1 also accepts operator_lookup parameter as well as other stages, or cut operator_lookup parameter from stage_2 as well as stage_1.

I don't know whether accepting operator_lookup parameter is important or not. Maybe the best advise to my feeling is to just ignore and walk away.

But I want to hear from original author/developer, about whether this is important for entire specs or just a tiny small thing I should pass through.

Possible impact

In clvm_tools/cmds.py, user can change stage by specifying -s or --stage option. And I found the code below, which switches run_program implemantation according to -s option.

    if hasattr(args.stage, "run_program_for_search_paths"):
        run_program = args.stage.run_program_for_search_paths(args.include)
    else:
        run_program = args.stage.run_program

And later, it is invoked without operator_lookup parameter.

            cost, result = run_program(
                run_script, input_sexp, max_cost=max_cost, pre_eval_f=pre_eval_f, strict=args.strict)

So , in current clvm_tools/cmds.py there is no issue at all.

But what if run_program above is invoked with operator_lookup parameter in future? If it is then, it becomse that stage_1 implementation of run_program ignores operator_lookup while stage_2's impl doesn't. Is this expected behaviour?

Thanks.

richardkiss commented 3 years ago

Stage 0 is an assembler for clvm.

Stage 1 was a first attempt at making it possible to write complex code that calls functions by name by adding a opcode bind to the language that allows additional opcodes to be added to the language dynamically, associating a function name with clvm code. An example is the factorial function here: https://github.com/Chia-Network/clvm_tools/blob/main/tests/stage_1/fact-1.txt

Stage 1 turned out to have drawbacks (namely, a second namespace that takes on python-like characteristics through which environment data can too easily be leaked), so it was a dead end. The code is still being maintained, but just barely, and should eventually be removed.

Stage 2 is what we generally call "the Chialisp compiler", and it supports compiling Chialisp into clvm. It adds com and opt (and a few others) as operators to clvm.

In general, you don't want to use non-standard values for operator_lookup when using run_program, and the API will change to reflect this at some point.

richardkiss commented 3 years ago

Comments added in https://github.com/Chia-Network/clvm_tools/pull/70