test: Add gettransaction test for "coin-join" tx

maflcko commented at 11:13 pm on May 8, 2020: member

fanquake added the label Tests on May 8, 2020

maflcko commented at 11:15 pm on May 8, 2020: member

Mostly for #14136 (and the ten other issues I closed with the same reported bug)

in test/functional/wallet_listtransactions.py:236 in fa592dc59b outdated

231+        )
232+        raw_hex = self.nodes[0].signrawtransactionwithwallet(raw_hex)['hex']
233+        raw_hex = self.nodes[1].signrawtransactionwithwallet(raw_hex)['hex']
234+        txid_join = self.nodes[0].sendrawtransaction(hexstring=raw_hex, maxfeerate=0)
235+        fee_join = self.nodes[0].getmempoolentry(txid_join)['fees']['base']
236+        #assert_equal(fee_join, self.nodes[0].gettransaction(txid_join)['fee'])

instagibbs commented at 6:35 pm on May 9, 2020:

hm?

maflcko commented at 6:39 pm on May 9, 2020:

That’s the bug. the # can be removed when the bug is fixed

maflcko commented at 12:10 pm on December 24, 2021:

Thanks, fixed comment to clarify

luke-jr commented at 6:40 pm on June 2, 2020: member

We would need major enhancements to support CoinJoin transactions. It’s a lot more involved than just showing the correct “fee” (which this test would not check correctly anyway - some of the fee may be going to a CJ peer instead of the miner).

maflcko commented at 7:19 pm on June 2, 2020: member

@luke-jr If this is unsupported, it should be documented as unsupported. See also my reply here: #19002 (comment)

luke-jr commented at 8:21 pm on June 2, 2020: member

Should we document every other thing that isn’t supported too?

Unsupported is the default…

DrahtBot commented at 9:20 pm on November 10, 2020: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy
Concept ACK	ismaelsadeeq

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label Needs rebase on Jun 24, 2021

maflcko force-pushed on Dec 24, 2021

DrahtBot removed the label Needs rebase on Dec 24, 2021

fanquake requested review from instagibbs on May 6, 2022

fanquake requested review from jonatack on May 6, 2022

in test/functional/wallet_listtransactions.py:306 in faf2d7dbb7 outdated

301+        raw_hex = self.nodes[0].signrawtransactionwithwallet(raw_hex)["hex"]
302+        raw_hex = self.nodes[1].signrawtransactionwithwallet(raw_hex)["hex"]
303+        txid_join = self.nodes[0].sendrawtransaction(hexstring=raw_hex, maxfeerate=0)
304+        fee_join = self.nodes[0].getmempoolentry(txid_join)["fees"]["base"]
305+        # Fee should be correct: assert_equal(fee_join, self.nodes[0].gettransaction(txid_join)['fee'])
306+        # But it is not:

instagibbs commented at 1:39 pm on May 6, 2022:

link the issue directly here so someone can follow-up with the rebased fix?

maflcko commented at 10:24 am on May 9, 2022:

Thanks, done

maflcko force-pushed on May 9, 2022

fanquake requested review from instagibbs on May 9, 2022

in test/functional/wallet_listtransactions.py:311 in fad82aef05 outdated

302+        raw_hex = self.nodes[1].signrawtransactionwithwallet(raw_hex)["hex"]
303+        txid_join = self.nodes[0].sendrawtransaction(hexstring=raw_hex, maxfeerate=0)
304+        fee_join = self.nodes[0].getmempoolentry(txid_join)["fees"]["base"]
305+        # Fee should be correct: assert_equal(fee_join, self.nodes[0].gettransaction(txid_join)['fee'])
306+        # But it is not, see for example https://github.com/bitcoin/bitcoin/issues/14136:
307+        assert fee_join != self.nodes[0].gettransaction(txid_join)["fee"]

kouloumos commented at 11:20 am on June 2, 2022:

As this tests confirms that a bug still exists for the gettransaction rpc call, would it make sense to further document the issue by adding another assertion from nodes[1] perspective? Because in the case that inputs are not the same, the resulting fee from gettransaction is different based on which side called the RPC.

maflcko commented at 6:09 am on June 10, 2022:

I think the test as-is now is sufficient

ismaelsadeeq commented at 4:51 pm on March 14, 2023: member

t(ACK) , I recreate and the bug exist also no need to add the node[1]’s perspective, I have tested and it’s all the same for both nodes.

maflcko force-pushed on Aug 23, 2023

maflcko force-pushed on Sep 14, 2023

maflcko commented at 5:28 am on September 15, 2023: member

rebased

test: Add gettransaction test for "coin-join" tx fa20f8919c

maflcko force-pushed on Nov 23, 2023

fanquake requested review from furszy on Nov 23, 2023

in test/functional/wallet_listtransactions.py:309 in fa20f8919c

304+        )
305+        raw_hex = self.nodes[0].signrawtransactionwithwallet(raw_hex)["hex"]
306+        raw_hex = self.nodes[1].signrawtransactionwithwallet(raw_hex)["hex"]
307+        txid_join = self.nodes[0].sendrawtransaction(hexstring=raw_hex, maxfeerate=0)
308+        fee_join = self.nodes[0].getmempoolentry(txid_join)["fees"]["base"]
309+        # Fee should be correct: assert_equal(fee_join, self.nodes[0].gettransaction(txid_join)['fee'])

furszy commented at 2:14 pm on November 23, 2023:

Could we add a “todo:” at the beginning? Just to make it easier to find broken things in the future.

maflcko commented at 2:28 pm on November 23, 2023:

I am not a fan of TODO comments, because I think issues in the issue tracker are a better way to track outstanding feature requests and bugfixes. An issue allows anyone to leave new context, ask questions, or look up the full discussion history on the topic.

So going to leave as-is for now.

furszy commented at 2:15 pm on November 23, 2023: member

utACK fa20f8919c4f

DrahtBot requested review from ismaelsadeeq on Nov 23, 2023

fanquake merged this on Nov 23, 2023

fanquake closed this on Nov 23, 2023

maflcko deleted the branch on Nov 24, 2023

test: Add gettransaction test for “coin-join” tx #18919

Code Coverage

Reviews

Conflicts