Now is not always now

[abitmore]

Bug Description
When processing a block, we usually use database::head_block_time() aka dynamic_global_properties.time to indicate "now". We also use database::head_block_num(). However, these variables are updated in the middle of the process, specifically, in database::update_global_dynamic_data(), after processed transactions:

bitshares-core/libraries/chain/db_block.cpp

Lines 520 to 524 in 44a7c47

	apply_transaction( trx, skip );
	++_current_trx_in_block;
	}
	update_global_dynamic_data(next_block);

It means when processing transactions, we're using the time and block number of last block. Technically it is not a big deal, but it has brought some confusions, for example:

• if an object (e.g. account or limit order) is created in a block by a transaction, its creation time and block number stored in object database are same as the values of previous block, but not current block;
• a transaction would have expired, or would be invalid due to other time-related reasons (e.g. hard fork time check), but still can be included in a block (Expired transactions with expiration time later than last block are possible to be included in a new block #356);
• a force-settlement will execute one block earlier;
• (perhaps) an order / proposal / withdraw_permission can be created in a block and then expire in the same block.

Impacts
Describe which portion(s) of BitShares Core may be impacted by this bug. Please tick at least one box.

• API (the application programming interface)
• Build (the build process or something prior to compiled code)
• CLI (the command line wallet)
• Deployment (the deployment process after building such as Docker, Travis, etc.)
• DEX (the Decentralized EXchange, market engine, etc.)
• P2P (the peer-to-peer network for transaction/block propagation)
• Performance (system or user efficiency, etc.)
• Protocol (the blockchain logic, consensus, validation, etc.)
• Security (the security of system or user data, etc.)
• UX (the User Experience)
• Other (please add below)

Expected Behavior
Block time and block number should be consistent when processing a block.

Additional Context (optional)
This change requires a consensus upgrade. Do we need to correct/update old data after the upgrade?

CORE TEAM TASK LIST

• Evaluate / Prioritize Bug Report
• Refine User Stories / Requirements
• Define Test Cases
• Design / Develop Solution
• Perform QA/Testing
• Update Documentation

[jmjatlanta]

Proposed solution: Add new properties in addition to head_block_time and head_block_num called current_block_time and current_block_num. These will be set to head_block_time and head_block_num unless you are processing a block. They should also be rolled back in the case of an exception that stops block processing.

Transaction and other processing that can happen within a block should use the new fields. Anything outside that process can continue to use head_block_???

Note: This can mean some metadata within things like transactions can change. i.e. a transaction being moved from pending to within a block will have their block time and number changed. I have not researched the implications of this, but believe it to be okay, as there are no guarantees that pending transactions will end up in a particular block.

[cogutvalera]

there are no guarantees that pending transactions will end up in a particular block, and there can be different reasons, not only due time, but also due to size.

[pmconrad]

Don't forget block generation. That currently uses head_block_time of the block that it is building upon.

I agree that the current implementation is not nice and may lead to surprising results sometimes, but it doesn't hurt very much in practice. Changing it requires significant effort and will forever pollute our codebase with a hardfork.

[jmjatlanta]

I agree that the current implementation is not nice and may lead to surprising results sometimes, but it doesn't hurt very much in practice. Changing it requires significant effort and will forever pollute our codebase with a hardfork.

After examining the issue, I must agree with @pmconrad. If all agree, we will mark this as a wontfix.

[abitmore]

Won't fix.