Don't allow getutxo answer to go over the size of the send buffer.

mikehearn commented at 3:42 pm on August 27, 2014: contributor

Limit the possible answer size for getutxo.

This does not adjust MAX_INV_SZ because it’s unnecessary for the mix, additionally, requesting normal outputs even up to MAX_INV_SZ does not run out of space in the send buffer. To make a giant response you need a giant output. So there might be some use case for doing a big query, at any rate, if the send buffer is too big then that’s a general issue we should fix elsewhere.

In the end I didn’t truncate the answer. I just return false which hits Misbehaving(). The reason is, again, no real app will ever hit this case. You need to deliberately pick a giant output and a giant number of repeated queries. So Misbehaving is a reasonable solution.

Don't allow getutxo answer to go over the size of the send buffer. dfb40dd0cd

jgarzik commented at 3:43 pm on August 27, 2014: contributor

Where are the tests that would have caught this in the first place? Have those tests been updated?

luke-jr commented at 3:45 pm on August 27, 2014: member

Any reason to allow >1? Seems like a good opportunity to discourage address reuse abuses.

mikehearn commented at 3:56 pm on August 27, 2014: contributor

Jeff, once again you are asking questions that were already answered in the description of the original patch. Please do read that document, but I’ll repeat the relevant section here:

Testing

I attempted to write unit tests for this, but Core has no infrastructure for building test chains …. the miner_tests.cpp code does it but at the cost of not allowing any other unit test to do so, as it doesn’t reset or clean up the global state afterwards! I tried to fix this and ended up down a giant rabbit hole.

So instead I’ve tested it with a local test app I wrote, which also exercises the client side part in bitcoinj.

The good news is, Sergio is making a start on fixing this, so in future we’ll be able to write unit tests that build test chains. The bad news is we still lack test infrastructure for testing the network stack - there are zero tests that cover ProcessMessages and as a result it’s difficult to write them. If you’d like to help fill that need that’d be fantastic.

Additionally, what dhill reported does not trigger a crash or any other kind of bug that could be easily found using unit testing. It doesn’t even seem to trigger a lot of network usage, I guess the data does get truncated somewhere inside the network stack. I repeated his test against my own bitcoind and it survived just fine. What it does is cause a spike in memory usage, so dhill ran such queries many times in parallel until his host ran out of memory.

But Bitcoin Core does not have a fixed memory budget or any kind of limit - it’s just “whatever the host happens to have at the time”. I’m not even sure if there’s any way to write a unit test that checks memory usage. If there was one, it’d lead to the question of what kind of memory usage limit would we select?

So testing this is not as trivial as it looks. It comes back to the entire anti-DoS question we’ve been debating for years. A full resource management infrastructure and ability to set resource budgets would be great, and it’d allow us to define this as a bug and test for it.

BitcoinPullTester commented at 3:56 pm on August 27, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4770_dfb40dd0cdb8f3e65a3422c55353550c45130930/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

jgarzik commented at 3:58 pm on August 27, 2014: contributor

That’s a dodge. Key phrase “that would have caught this”

It is clearly possible to test that a P2P message does not return oversized output.

mikehearn commented at 4:05 pm on August 27, 2014: contributor

Well, like I said, at the moment I’m not sure how to write a test for it and there are no examples to guide me. I tried to test ProcessGetUTXOs independently (that’s why it’s a separate function) but until Sergio’s work lands there’s no way to build test chains, so I didn’t succeed.

Once there’s the right infrastructure for writing this kind of unit test then I’d be happy to do so. I did write functionality tests in the pull tester.

in src/main.cpp: in dfb40dd0cd

3584@@ -3577,6 +3585,9 @@ bool ProcessGetUTXOs(const vector<COutPoint> &vOutPoints, bool fCheckMemPool, ve
3585                     coin.nHeight = coins.nHeight;
3586                     coin.out = coins.vout.at(vOutPoints[i].n);
3587                     assert(!coin.out.IsNull());
3588+                    bytes_used += coin.GetSizeOf();

sipa commented at 4:10 pm on August 27, 2014:

You can use:

0bytes_used += ::GetSerializeSize(coin, SER_NETWORK, PROTOCOL_VERSION);

instead of defining a custom size computer.

in src/main.cpp: in dfb40dd0cd

3559@@ -3555,6 +3560,9 @@ bool ProcessGetUTXOs(const vector<COutPoint> &vOutPoints, bool fCheckMemPool, ve
3560 
3561     LogPrint("net", "getutxos for %d queries %s mempool\n", vOutPoints.size(), fCheckMemPool ? "with" : "without");
3562 
3563+    // Due to the above check max space the bitmap can use is 50,000 / 8 == 6250 bytes.
3564+    size_t bytes_used = vOutPoints.size() / 8;
3565+    size_t max_bytes = SendBufferSize();

sipa commented at 4:11 pm on August 27, 2014:

So decreasing the send buffer size will increase the chance that we start disconnecting peers?

mikehearn commented at 4:16 pm on August 27, 2014:

How so? Like I said, requesting 50,000 outputs of a normal output would not even trigger problems. Requesting 50,000 outputs of the biggest output on the testnet doesn’t even kill my node, it just bloats up and then shrinks a few seconds later. dhill had to run many times in parallel to get the effect of him running out of RAM.

So I don’t see why shrinking the send buffer would cause problems for any normal app, not even if we halved it.

btcdrak commented at 4:11 pm on August 27, 2014: contributor

Seems like yet another reason to revert #4351 pending proper investigation and Sergio’s patch to enable tests to be written. There is clearly zero urgency for this and it needs more time and consideration to find the right solution (or even decide if it should be a feature of p2p at all). There is no reason you can’t write this for bitcoinj afterall…

laanwj added the label Improvement on Aug 28, 2014

jgarzik commented at 2:50 pm on September 14, 2014: contributor

Closing due to #4351 revert. Presumably this gets rolled into the main getutxos implementation.

jgarzik closed this on Sep 14, 2014

MarcoFalke locked this on Sep 8, 2021

Don’t allow getutxo answer to go over the size of the send buffer. #4770