rpcclient: map[btcutil.Address]btcutil.Amount in sendmany is a pretty dangerous

[davecgh]

From @glenherb on January 18, 2017 17:29

For example take this code:

amounts := make(map[btcutil.Address]btcutil.Amount)
for _, p := range paymentsToMake {

  addr, _ := btcutil.DecodeAddress(p.Address, net)
  amounts[addr] = amounts[addr] + p.amount
}
client.SendMany("", amounts)

There is a subtle problem that looking up in amounts will never work, because despite btcutil.Address being comparable, it's not a logic comparison. When it's finally sent over rpc, one of the payments can be silently be dropped

Copied from original issue: btcsuite/btcrpcclient#112

[davecgh]

From @jrick on January 18, 2017 17:40

Yeah, this is an issue because we set the pointers to the concrete address type as the Address interface value. If we put the non-pointer type in the interface I believe it will equate correct, since it will compare the actual struct values themselves instead of doing pointer equality.

[davecgh]

From @jrick on January 18, 2017 17:45

Sadly that's not possible since we are using pointer receivers, so the values themselves cannot satisfy the interface: https://play.golang.org/p/J7UWiO7x_o

(change the pointer receiver to a non-pointer and notice how the commented out code now compiles, and evaluates to true.)

[davecgh]

From @jrick on January 18, 2017 17:55

We are still stuck with this data structure though since it has to encode to a json object for the bitcoin json-rpc api. In my mind it makes more sense to provide an (ordered) array of address+value objects to add as outputs for the transaction, but this would be incompatible with the existing api.

[davecgh]

From @FrozenPrincess on January 21, 2017 6:2

This seems like a pretty simple fix, no?

The function is currently:

func (c *Client) SendManyAsync(fromAccount string, amounts map[btcutil.Address]btcutil.Amount) FutureSendManyResult {
	convertedAmounts := make(map[string]float64, len(amounts))
	for addr, amount := range amounts {
		convertedAmounts[addr.EncodeAddress()] = amount.ToBTC()
	}
	cmd := btcjson.NewSendManyCmd(fromAccount, convertedAmounts, nil, nil)
	return c.sendCmd(cmd)
}

and just needs to be changed to:

func (c *Client) SendManyAsync(fromAccount string, amounts map[btcutil.Address]btcutil.Amount) FutureSendManyResult {
	convertedAmounts := make(map[string]float64, len(amounts))
	for addr, amount := range amounts {
                encodedAddress := addr.EncodeAddress()
		convertedAmounts[encodedAddress] = convertedAmounts[encodedAddress] + amount.ToBTC()
	}
	cmd := btcjson.NewSendManyCmd(fromAccount, convertedAmounts, nil, nil)
	return c.sendCmd(cmd)
}

[davecgh]

From @jrick on January 21, 2017 16:54

I'm not happy with that either because two outputs both paying to the same address is not the same thing as a single payment of the combined amount.

While the API here is pretty stupid, this is actually not technically a problem with the fact that bitcoin uses a JSON object keyed by addresses for representing these, as JSON does allow (but discourages) duplicate keys. It becomes a problem for us (and implementations in many other languages) that use a hashmap/btreemap/etc. for representation of the object.

I also think the original issue was talking more about the general fact that map[btcutil.Address]foo is generally a bad idea, or that you must be very careful using them. But then again, it's no more dangerous than comparing btcutil.Address objects already, since that will also just do a memory address comparison.

[davecgh]

From @FrozenPrincess on January 21, 2017 17:6

While the API here is pretty stupid, this is actually not technically a problem with the fact that bitcoin uses a JSON object keyed by addresses for representing these, as JSON does allow (but discourages) duplicate keys.

Well, it's technically a problem in the sense it's currently not working. I just tested (against bitcoin core) with a map[btcutil.Address]btcutil.Amount with two same-but-different addresses. When it sent a transaction, one of the entries overwrote the other.

		convertedAmounts[addr.EncodeAddress()] = amount.ToBTC()

Is blasting away the previous entry.

I'd also think it's acceptable to check the convertedAmounts before writing, and give an error if an entry already exist. Any behavior is better than silently overwriting

[Infra1515]

Greetings, I am getting this exact behaviour when I am trying to send to the same address two different times using SendMany(). The second entry and it's amount is getting overwritten

I tried to check if the address is in the map before and increment the amount so it will go into one output but using btcutil.Address generated via btcutil.DecodeAddress as key always yields a different a unique key in the map ( memory address comparison)

if _, ok := destinationAddressesWithAmount[nftPayoutAddress]; ok { // if the address is already there then increment the amount it will receive 
					destinationAddressesWithAmount[nftPayoutAddress] += payoutAmount

				} else {
					destinationAddressesWithAmount[nftPayoutAddress] = payoutAmount
				}

I read from above that this is a limitation of bitcoin due to that bitcoin uses a JSON object keyed by addresses for representing adresses, as JSON does allow (but discourages) duplicate keys and I understand this.
In any case there should be a warning somewhere in the docstring that if you pass the same address two times, it would be overwritten.
Also it would be nice if it was added another function/overload that would do a single payment of the combined amount as mentioned in the above comments - something similar to this ( as mentioned above by @FrozenPrincess )

func (c *Client) SendManyAsync(fromAccount string, amounts map[btcutil.Address]btcutil.Amount) FutureSendManyResult {
	convertedAmounts := make(map[string]float64, len(amounts))
	for addr, amount := range amounts {
                encodedAddress := addr.EncodeAddress()
		convertedAmounts[encodedAddress] = convertedAmounts[encodedAddress] + amount.ToBTC()
	}
	cmd := btcjson.NewSendManyCmd(fromAccount, convertedAmounts, nil, nil)
	return c.sendCmd(cmd)
}

Best regards !