Fix finalize print statement #2976
Conversation
synthesizer/process/src/finalize.rs
Outdated
| let constructor_types = stack.get_constructor_types()?.clone(); | ||
|
|
||
| // Set the FinalizeRegisters function name for constructors. | ||
| // Before ConsensusVersion::V12, this was wrongly using the program id as function name. |
There was a problem hiding this comment.
Why is it "wrong" to use the program ID name vs. hard-coding constructor? Is there a security issue or is this purely for logging?
There was a problem hiding this comment.
Wrong was maybe strongly worded, but we are setting a program_id into a variable called function_name, which is semantically wrong. It's a piece of the code few people looked at, so added some documentation: f644a54
synthesizer/process/src/finalize.rs
Outdated
| // Set the FinalizeRegisters function name for constructors. | ||
| // Before ConsensusVersion::V12, this was wrongly using the program id as function name. | ||
| let consensus_version = N::CONSENSUS_VERSION(state.block_height())?; | ||
| let identifier = if consensus_version >= ConsensusVersion::V12 { |
There was a problem hiding this comment.
If the purpose of this change is to improve the log message, can you pass in a flag to finalize_command_expect_await and set the message accordingly?
This avoids the permanent "overhead" of tracking this to a ConsensusVersion
There was a problem hiding this comment.
Agreed, it adds complexity and risks a fork. Changed it to just a logging adjustment: 7d92c1e
vicsn
force-pushed
the
fix_finalize_print
branch
from
895b850 to
7d92c1e
Compare
Motivation
When investigating the reasons for a rejected transaction, I surfaced these logs:
Based on the respective commands which failed, it is clear these are 'transfer_public' and 'fee_public' calls which fail due to insufficient balance. Instead of logging these as constructor calls, they should be logged as the respective function call.
Test Plan
Manually reviewed all instances of
registers.function_nameto catch potential issues.