Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EVM event does not emits correctly same Event called twice on same function call #600

Closed
eduardonunesp opened this issue Jan 9, 2019 · 4 comments
Assignees

Comments

@eduardonunesp
Copy link
Contributor

eduardonunesp commented Jan 9, 2019

The issue was reported by E11, according to Allejo

Bug

The bug happens when a Contract with an Event is called twice inside a function body, the Event is only emitted one time, let's say we have a Contract like the following:

contract SimpleStore {
  uint256 public value;

  event NewValueSet(uint256 indexed _value);

  function set(uint256 _value) public {
    value = _value;
    emit NewValueSet(value);
    emit NewValueSet(value + 1);
  }
}

If a transaction was sent to the SimpleStore contract on function set with value equals 1, only one event will be emitted with the value equals 1, and the second event with value equals 2 isn't emitted

Expected

If a transaction was sent to the SimpleStore contract on function set with a value equals 1, two events should be emitted a first one with the value of 1 and a second with the value of 2

A proof of concept can be found at: loomnetwork/loom-js#194

@eduardonunesp eduardonunesp changed the title EVM event does not emits correctly same Event with different values EVM event does not emits correctly same Event with different values on same function call Jan 9, 2019
@eduardonunesp eduardonunesp changed the title EVM event does not emits correctly same Event with different values on same function call EVM event does not emits correctly same Event called twice on same function call Jan 9, 2019
@enlight
Copy link
Contributor

enlight commented Jan 10, 2019

I'm 90% sure the issue is here:

func (h *EthResetHub) Publish(message pubsub.Message) int {
	h.mutex.RLock()
	defer h.mutex.RUnlock()

	count := 0
	// iterate over all subscribers, and publish messages
	for sub, unsent := range h.registry {
		if unsent {
			if sub.Match(message.Topic()) {
				count += sub.Publish(message)
				h.registry[sub] = false
			}
		}
	}

	return count
}

So once EthResetHub.Publish sends a message to a particular subscriber it won't send any other message to the subscriber until EthResetHub.Reset is called and h.registry[sub] is set to true again to indicate it hasn't received a message.

The event handler does attempt to dispatch all events:

func (ed *DefaultEventHandler) EmitBlockTx(height uint64) (err error) {
	defer func() {
		if r := recover(); r != nil {
			err = fmt.Errorf("caught panic publishing event: %v", r)
		}
	}()
	msgs, err := ed.stash.fetch(height)
	if err != nil {
		return err
	}
	ed.ethSubscriptions.Reset()
	for _, msg := range msgs {
		emitMsg, err := json.Marshal(&msg)
		if err != nil {
			log.Default.Error("Error in event marshalling for event: %v", emitMsg)
		}
		eventData := types.EventData(*msg)
		ethMsg, err := proto.Marshal(&eventData)
		if err != nil {
			log.Default.Error("Error in event marshalling for event: %v", emitMsg)
		}

		log.Debug("sending event:", "height", height, "contract", msg.PluginName)
		if err := ed.dispatcher.Send(height, emitMsg); err != nil {
			log.Default.Error("Error sending event: height: %d; msg: %+v\n", height, msg)
		}
		contractTopic := "contract:" + msg.PluginName
		ed.subscriptions.Publish(pubsub.NewMessage(contractTopic, emitMsg))
		ed.ethSubscriptions.Publish(pubsub.NewMessage(string(ethMsg), emitMsg))
		for _, topic := range msg.Topics {
			ed.subscriptions.Publish(pubsub.NewMessage(topic, emitMsg))
			log.Debug("published WS event", "topic", topic)
		}
	}
	ed.stash.purge(height)
	return nil
}

@Sriep
Copy link
Contributor

Sriep commented Jan 10, 2019

I believe this feature was introduced to fix a previous issue where the same event was being emitted for matching both account and topics. Should be able to add to #463 or fix in current master.

@eduardonunesp
Copy link
Contributor Author

The issue above also happens when the function emits more than one event on the same function, like the issue described on loomnetwork/loom-js#146 also reproduced on loomnetwork/loom-js#194

@eduardonunesp
Copy link
Contributor Author

Thanks for the fix @pathornteng, the loomjs PR is ready to merge loomnetwork/loom-js#194

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants