test: Add multiple transactions and error handling tests for getreceivedbyaddress by danielabrozzoni · Pull Request #8 · danielabrozzoni/b4os-bitcoin

danielabrozzoni · 2026-03-02T13:36:58Z

This PR adds comprehensive functional test coverage for the getreceivedbyaddress RPC method.

- Add test for multiple transactions to same address - Add test for invalid address format error

danielabrozzoni · 2026-03-02T18:36:04Z

Review checklist

These steps can be useful to review any PR!

Start with the "why": What is this PR doing, and why does it matter? Skim the PR description first to get the high-level intent. Is this a good idea at all? If you're not sure this is a good idea, or you have doubts, you can just halt the review and ask, or NACK!
Configure + build, then run all tests locally. Sometimes people think "oh, CI already does that!"... It does, but in Bitcoin we always say "don't trust, verify", and also some tests might be flaky and you might catch that. So, build and test :)

    # configure
    cmake -B build
    # build
    cmake --build build
    # run the unit tests
    ./build/bin/test_bitcoin
    # run functional tests
    ./build/test/functional/test_runner.py

Review commit-by-commit: For each commit, read the commit message and description first, then review the diff.
Explore surrounding code:
- If you are reviewing a test change, jump to the relevant C++ code to understand what the test is exercising.
- Otherwise, explore the C++ code around the areas touched by the PR.

PR specific questions

These are some questions to guide through the review of this specific PR :)

The first thing to notice is that the PR description is not very accurate. Some useful context exists in the commit message, but it is easy to miss. Please leave a comment proposing a clearer description so that someone opening the PR immediately understands what the change does and why it exists.
Take some time to explore the C++ code exercised by this functional test. Here are some questions to guide you!
1. What parameters do getreceivedbyaddress and getreceivedbylabel accept, and which underlying function do they both call?
2. Can I call getreceivedbyaddress with any address existing in the blockchain? Or does the address need to be... something something specific, not just ANY address!
3. How does GetReceived work? How is it possible that it works both for received by label and address? What's the trick there?
4. In the final part of GetReceived, we call GetTxDepthInMainChain. Why? What does that return? How is depth used? You should reach a point where you can explain the whole condition in the if (https://github.com/danielabrozzoni/b4os-bitcoin/blob/master/src/wallet/rpc/coins.cpp#L61-L64)
There is a bug in the test. If you run it locally, it fails. Identify the reason for the failure, and leave a comment proposing a fix.
The commit organization is fine overall, but could be improved. Bitcoin Core developers follow the committing guidelines described in CONTRIBUTING.md. Can you identify ways the commit structure or messages could better follow these guidelines? If so, leave a comment describing the changes you would suggest.

xyzconstant · 2026-03-04T13:29:55Z

+        self.nodes[0].sendtoaddress(addr_with_multiple_txs, Decimal("0.2"))
+        self.generatetoaddress(self.nodes[0], 1, addr_with_multiple_txs)
+        self.generate(self.nodes[0], 1)
+        balance = self.nodes[1].getreceivedbyaddress(addr_with_multiple_txs, 5, True)


min confirmation set to 5, and only 2 blocks has been mined. This is making the test to break

bilthon · 2026-03-04T13:50:44Z

+        addr_with_multiple_txs = self.nodes[1].getnewaddress()
+        self.nodes[0].sendtoaddress(addr_with_multiple_txs, Decimal("0.1"))
+        self.nodes[0].sendtoaddress(addr_with_multiple_txs, Decimal("0.2"))
+        self.generatetoaddress(self.nodes[0], 1, addr_with_multiple_txs)


Remove this and just call generate with 5 blocks. Otherwise the test fails.

talkingmeat · 2026-03-04T13:47:14Z

                            received_by_label_json)

-        # getreceivedbyaddress should return same balance because of 0 confirmations
+        # getreceivedbylabel should return same balance because of 0 confirmations


This is a necessary cleanup, but don't forget to add it in the PR description.
Also, it could be nice to put preparatory cleanups on a separate commit.

talkingmeat · 2026-03-04T13:57:33Z

+        self.nodes[0].sendtoaddress(addr_with_multiple_txs, Decimal("0.2"))
+        self.generatetoaddress(self.nodes[0], 1, addr_with_multiple_txs)
+        self.generate(self.nodes[0], 1)
+        balance = self.nodes[1].getreceivedbyaddress(addr_with_multiple_txs, 5, True)


This has two issues:

first, min_confirmations is set to 5, and only 2 blocks were mined, this will make the total balance 0, failing the test.

second, even if the first issue is fixed, the include_immature_coinbase is set to True; this causes the total balance to be 25.3 + fee instead of 0.3 since node[1] is receiving the coinbase amount in addr_with_multiple_txs from node[0]'s mining.

Make sure tests in CI are all passing!

This has two issues:

first, min_confirmations is set to 5, and only 2 blocks were mined, this will make the total balance 0, failing the test.

second, even if the first issue is fixed, the include_immature_coinbase is set to True; this causes the total balance to be 25.3 + fee instead of 0.3 since node[1] is receiving the coinbase amount in addr_with_multiple_txs from node[0]'s mining.

Make sure tests in CI are all passing!

Can confirm, tested locally and this is exactly what happened.

qatkk

Agree with the previous comments. Just a small take on how the commits shall be done.

qatkk · 2026-03-04T14:17:54Z

+        assert_equal(balance, Decimal("0.3"))
+
+        # Test invalid address format error
+        assert_raises_rpc_error(-5, "Invalid Bitcoin address", self.nodes[1].getreceivedbyaddress, "invalid_address")


Maybe it would make sense to have this change in a separate commit for each commit to address only one of the issues presented in the goals of the PR.

Dmenec · 2026-03-04T14:22:40Z

Hey! I think maybe the description could be a little bit clearer.
What do you think about changing it to:

This test covers the functionality for handling multiple transactions sent to the same address and verifies address formatting in the getreceivedbyaddress RPC method.

Also maybe you can split up the commits as @qatkk mentions and add a specific description for both.

In any case, it still has to address the comments made by other reviewers to fix the errors in the code.

Also, please remember to run the tests locally before pushing to make sure everything passes :)

0xlaga

Review of PR #8: test: Add multiple transactions and error handling tests for getreceivedbyaddress

Concept ACK

The idea of adding test coverage for getreceivedbyaddress (multi-tx accumulation, invalid address error) and fixing the comment typo is good. However, the test has a bug and doesn't pass locally. See below.

1. PR Description

The current description — "This PR adds comprehensive functional test coverage for the getreceivedbyaddress RPC method" — is too vague. A reader opening the PR can't tell what's actually changing without clicking into the diff. The commit message has better context but is easy to miss.

Suggested description:

Add two new test cases to the getreceivedbyaddress section of wallet_listreceivedby.py:

Multiple transactions to the same address: verify that getreceivedbyaddress correctly sums the amounts from two separate sends to a single address.

Invalid address error: verify that passing a malformed address string returns the expected RPC error (-5, "Invalid Bitcoin address").

Also fix an incorrect comment that referenced getreceivedbyaddress where the code actually calls getreceivedbylabel.

2. Bug in the Test

Running the test locally, it fails:

AssertionError: not(0E-8 == 0.3)

at line 134. There are two interacting problems in this code:

self.generatetoaddress(self.nodes[0], 1, addr_with_multiple_txs)  # (A)
self.generate(self.nodes[0], 1)
balance = self.nodes[1].getreceivedbyaddress(addr_with_multiple_txs, 5, True)  # (B)
assert_equal(balance, Decimal("0.3"))

(B) Primary failure — minconf=5 but only 2 confirmations:
Only 2 blocks are mined after the two sendtoaddress calls, so depth=2 < min_depth=5 and GetReceived skips the transactions entirely → returns 0.

(A) Hidden bug — coinbase reward to the test address:
generatetoaddress mines a block whose 25 BTC coinbase reward goes to addr_with_multiple_txs. Since include_immature_coinbase=True is passed, after fixing (B) the balance would be 25.3, not 0.3.

Suggested fix: use self.generate (no coinbase to the test address) and use the default minconf=1:

# Test multiple transactions to the same address
addr_with_multiple_txs = self.nodes[1].getnewaddress()
self.nodes[0].sendtoaddress(addr_with_multiple_txs, Decimal("0.1"))
self.nodes[0].sendtoaddress(addr_with_multiple_txs, Decimal("0.2"))
self.generate(self.nodes[0], 1)
balance = self.nodes[1].getreceivedbyaddress(addr_with_multiple_txs)
assert_equal(balance, Decimal("0.3"))

3. Commit Organization

Per CONTRIBUTING.md, each commit should be a single logical change. This PR bundles three things into one commit:

New test: multiple transactions to the same address
New test: invalid address error path
Unrelated comment typo fix (getreceivedbyaddress → getreceivedbylabel)

The typo fix should be a separate commit since it's logically independent:

Commit 1: doc: Fix incorrect RPC name in wallet_listreceivedby comment
Commit 2: test: Add getreceivedbyaddress tests for multi-tx accumulation and invalid address

danielabrozzoni · 2026-03-04T18:05:30Z

Closing the PR after the workshop is finished, thank you everyone for interacting with it! :)

danielabrozzoni force-pushed the test_getreceivedbyaddress branch from d45ec3f to fc2eb57 Compare March 2, 2026 14:04

test: Add getreceivedbyaddress coverage to wallet_listreceivedby

9c68325

- Add test for multiple transactions to same address - Add test for invalid address format error

danielabrozzoni force-pushed the test_getreceivedbyaddress branch from fc2eb57 to 9c68325 Compare March 2, 2026 14:52

danielabrozzoni added B4OS Review Club labels Mar 2, 2026

xyzconstant reviewed Mar 4, 2026

View reviewed changes

bilthon reviewed Mar 4, 2026

View reviewed changes

talkingmeat reviewed Mar 4, 2026

View reviewed changes

qatkk reviewed Mar 4, 2026

View reviewed changes

0xlaga suggested changes Mar 4, 2026

View reviewed changes

danielabrozzoni closed this Mar 4, 2026

Navigation Menu

test: Add multiple transactions and error handling tests for getreceivedbyaddress - #8

test: Add multiple transactions and error handling tests for getreceivedbyaddress#8
danielabrozzoni wants to merge 1 commit into
test_getreceivedbyaddress_masterfrom
test_getreceivedbyaddress

danielabrozzoni commented Mar 2, 2026

Uh oh!

danielabrozzoni commented Mar 2, 2026

Uh oh!

xyzconstant Mar 4, 2026 •

edited

Loading

Uh oh!

bilthon Mar 4, 2026

Uh oh!

talkingmeat Mar 4, 2026

Uh oh!

talkingmeat Mar 4, 2026 •

edited

Loading

Uh oh!

xyzconstant Mar 4, 2026

Uh oh!

qatkk left a comment

Uh oh!

qatkk Mar 4, 2026

Uh oh!

Dmenec commented Mar 4, 2026 •

edited

Loading

Uh oh!

0xlaga left a comment

Uh oh!

danielabrozzoni commented Mar 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

danielabrozzoni commented Mar 2, 2026

Uh oh!

danielabrozzoni commented Mar 2, 2026

Review checklist

PR specific questions

Uh oh!

xyzconstant Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bilthon Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

talkingmeat Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

talkingmeat Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xyzconstant Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

qatkk left a comment

Choose a reason for hiding this comment

Uh oh!

qatkk Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Dmenec commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xlaga left a comment

Choose a reason for hiding this comment

Review of PR #8: test: Add multiple transactions and error handling tests for getreceivedbyaddress

Concept ACK

1. PR Description

2. Bug in the Test

3. Commit Organization

Uh oh!

danielabrozzoni commented Mar 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

xyzconstant Mar 4, 2026 •

edited

Loading

talkingmeat Mar 4, 2026 •

edited

Loading

Dmenec commented Mar 4, 2026 •

edited

Loading