Filter out phantom txs from test_observer::parse_transaction fn

[jferrant]

Closes #5634

We now generate phantom unlock txs and include them in tenure change blocks. These really are actually only tx receipts though not actual transactions. I attempt to filter them out from the test_observer call. I could prevent them being added to the transaction list but I feel like maybe that its better to include them and just not parse them out from the final test_observer end.

[jcnelson]

IIRC we actually want the first block in a tenure to only include the tenure-change and coinbase transactions, because we want it to be as quickly propagated and processed as possible so as to maximize the chances that the next miner can confirm it with its block-commit.

[jferrant]

IIRC we actually want the first block in a tenure to only include the tenure-change and coinbase transactions, because we want it to be as quickly propagated and processed as possible so as to maximize the chances that the next miner can confirm it with its block-commit.

This is only really true for starting a tenure. For tenure changes that are time based we don't actually care. We also don't enforce this anywhere. I think its okay to have in the test because no where in consensus (by the signers or block validation) do we enforce the first block to only contain these two transaction types. We do enforce that they are the FIRST transactions in the block when expected thought. I agree it would be dumb of a miner to needlessly fill the block with token transfers or any other transactions for that miner when initializing their tenure, but its not against the rules to do so. Unless there is a way for me to differentiate a token transfer from a phantom tx vs regular token transfer...not sure how to do this...I dont actually think its a bad thing to check this in the tests.

EDIT: but looking closer at the phantom tx...it only produces a receipt so perhaps this is obfuscating a deeper issue. Will look into !

EDIT 2: @obycode you were right. This seemed to be an issue on the test_observer side that would parse out the transaction when it shouldn't necessarily. I could prevent them from being included in the orignal transaction list or add a flag to the list of "is this a phantom tx" but I think having it at the lower level test_observer::parse_transactions call is prob best in case some test wants to actually use this data.

[hstove]

I feel like we should either:

• Add a doc comment to parse_transactions to indicate that this filters out the phantom unlock events (actually all unlock events, not just "phantom" ones)
• Just update check_nakamoto_empty_block_heuristics to do this filter

I'd probably lean towards option 2 but I'm ok either way!

[jferrant]

I feel like we should either:

* Add a doc comment to `parse_transactions` to indicate that this filters out the phantom unlock events (actually all unlock events, not just "phantom" ones)

* Just update `check_nakamoto_empty_block_heuristics` to do this filter

I'd probably lean towards option 2 but I'm ok either way!

They are phantom ones only as far as I can tell because I do believe only phantom tx's submit an amount of 0 to a boot code address...but not 100% sure on this... @jcnelson do you know if there is any other instance where we would have a token transfer with these in place?

[jcnelson]

LGTM. What you have is fine for detecting phantom transactions, but please just add a comment about them (also, note that they are indistinguishable from STX allocation "transactions" that get processed in the genesis block). It may be a good idea in the future to add a function is_phantom_transaction() to StacksTransaction at some point.

[jferrant]

LGTM. What you have is fine for detecting phantom transactions, but please just add a comment about them (also, note that they are indistinguishable from STX allocation "transactions" that get processed in the genesis block). It may be a good idea in the future to add a function is_phantom_transaction() to StacksTransaction at some point.

Done! Also added the is_phantom function.

[blockstack-devops]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.