Check if a proposal is alive before removing #1481
Conversation
| // then be re-inserted to database when undo session destructs, | ||
| // in this case the address would be different, which means the reference would be invalidated already. | ||
| const proposal_object* po = find(proposal_id); | ||
| FC_ASSERT( po, "Proposal ${p} was removed unexpectedly", ("p",proposal_id) ); // make sure the proposal still exists |
There was a problem hiding this comment.
If this assertion throws the session will be rolled back, which reconstructs the proposal.
This means the enclosing proposal_update succeeds, which leads to the proposal being removed again.
This will cause the enclosing push_proposal to throw at the same point, which again reconstructs it.
This alternating success/failure looks dirty to me. Does it mean the ultimate success or failure depends on the nesting depth?
There was a problem hiding this comment.
Actually this is generic defensive logic -- only tries to remove an object if it exists. In addition, to be defensive, locate the object again. The rationale is a proposal can be removed and then reconstructed not only due to self-approving (I believe we can make a test case for this). I do agree the self-approving proposal makes the situation more complicated, and we do need to avoid the situation in the first place as #1480 tries to do.
There was a problem hiding this comment.
a proposal can be removed and then reconstructed not only due to self-approving (I believe we can make a test case for this)
Update:
I may be wrong. When pushing a proposal, the proposal itself should not be removed if we've avoided self-approving as well as loops.
| @@ -156,6 +156,8 @@ void database::clear_expired_proposals() | |||
| while( !proposal_expiration_index.empty() && proposal_expiration_index.begin()->expiration_time <= head_block_time() ) | |||
| { | |||
| const proposal_object& proposal = *proposal_expiration_index.begin(); | |||
| proposal_object proposal_copy = proposal; // make a copy for logging | |||
| proposal_id_type proposal_id = proposal.id; | |||
There was a problem hiding this comment.
Can use proposal_copy.id instead of introducing proposal_id
| @@ -229,7 +229,9 @@ void_result proposal_update_evaluator::do_evaluate(const proposal_update_operati | |||
| } FC_CAPTURE_AND_RETHROW( (o) ) } | |||
|
|
|||
| void_result proposal_update_evaluator::do_apply(const proposal_update_operation& o) | |||
| { try { | |||
| { | |||
| auto o_copy = o; // make a copy for logging | |||
There was a problem hiding this comment.
Is that necessary? o lives longer than o_copy.o can be removed, which deletes o.
There was a problem hiding this comment.
do we really need self-approving proposals ? maybe we should not accept & apply self-approving proposals ?
There was a problem hiding this comment.
@cogutvalera #1480 rejects self-approving proposals.
|
@abitmore do you think this is still worth changing? If not please close. |
Another attempt for issue #1479.