fix: include txid in more failure logs

[janniks]

Description

Adds txid to more failure logs for better interpretability of logs in production.

Additional info (benefits, drawbacks, caveats)

Can be used for better understanding what happened to a tx given a txid and logs.

Checklist

• Test coverage for new or modified code paths
• Changelog is updated
• Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
• New clarity functions have corresponding PR in clarity-benchmarking repo
• New integration test(s) added to bitcoin-tests.yml

[codecov]

Codecov Report

Attention: Patch coverage is 11.62791% with 38 lines in your changes are missing coverage. Please review.

Project coverage is 26.37%. Comparing base (aaaca77) to head (3fff50b).
Report is 175 commits behind head on develop.

❗ Current head 3fff50b differs from pull request most recent head 86dd3df. Consider uploading reports for the commit 86dd3df to get more accurate results

Files	Patch %	Lines
stackslib/src/chainstate/stacks/db/transactions.rs	11.90%	37 Missing ⚠️
stackslib/src/chainstate/stacks/miner.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #4396       +/-   ##
============================================
- Coverage    56.00%   26.37%   -29.63%     
============================================
  Files          465      404       -61     
  Lines       336186   293090    -43096     
============================================
- Hits        188266    77290   -110976     
- Misses      147920   215800    +67880     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jcnelson]

Hey, we don't open PRs into master directly unless it's a hotfix or a release. Can you instead open this against develop?

[janniks]

Apologies, forgot to switch 👍🏻 Will update

[jcnelson]

Just make sure CI passes

[janniks]

Right now tests are failing because some auto-generated contract names are too long. e.g. a contract name from setting up the test observer; blockstack_lib-chainstate-stacks-boot-pox_3_tests-pox_3_getters
Unfortunately, Txid serializes and panics on failure.

@jbencin any ideas on how to go about this, or point me to the correct code. e.g. if the exact string isn't important, we can slice it to the max contract length, or something similar.

[jbencin]

@jcnelson @janniks I found the issue. ContractName is implemented using a macro, like this:

guarded_string!(
    ContractName,
    "ContractName",
    CONTRACT_NAME_REGEX,
    MAX_STRING_LEN,
    RuntimeErrorType,
    RuntimeErrorType::BadNameValue
);

MAX_STRING_LEN is 128, so it's possible to construct a ContractName with up to 128 characters. However, when we implement StacksMessageCodec for ContractName:

    fn consensus_serialize<W: Write>(&self, fd: &mut W) -> Result<(), codec_error> {
        if self.as_bytes().len() < CONTRACT_MIN_NAME_LENGTH as usize
            || self.as_bytes().len() > CONTRACT_MAX_NAME_LENGTH as usize
        {
            // Error!
        }
        // ...
    }

CONTRACT_MAX_NAME_LENGTH is 40, much smaller than MAX_STRING_LEN. This means that ContractName::try_from() allows us to create structs that can't be serialized

Proposed solutions

• Change the macro to use CONTRACT_MAX_NAME_LENGTH instead of MAX_STRING_LEN. This won't actually fix the problem, and will likely expose more invalid ContractNames that were never being serialized, but it will make invalid ContractNames easy to find and fix. Where it fails, we can replace ContractName::try_from() with ContractName::try_from_truncated()
• Change CONTRACT_MAX_NAME_LENGTH to match MAX_STRING_LEN. Probably not what we want to do, but would fix the issue immediately

[janniks]

Updated the test helpers to not allow invalid contract name generation. (Capping at CONTRACT_MAX_NAME_LENGTH and remove leading dashes). This way tests shouldn't fail due to the logging of txids, while in production no unserializable txs should reach this code.

[jbencin]

Unnecessary

Suggested change

.to_string()

[jbencin]

Why not do this before limiting the length?

[jcnelson]

@jbencin @janniks This finding belongs in an issue and should be tackled by a separate PR. Thanks! 🙏

[jbencin]

@jcnelson Already made an issue, #4727

We are modifying the PR to avoid the issue for now