p2p: unbroadcast followups: rpcs, nLastResend, mempool sanity check

glozow commented at 0:38 am on May 6, 2020: member

Followup to #18038 by amitiuttarwar which introduces the unbroadcast set: “a mechanism for the mempool to track locally submitted transactions” and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.

This PR addresses some of the outstanding TODOs building on top of it:

remove nLastResend logic, which is used to ensure rebroadcast doesn’t happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (#18038 comment)
expose unbroadcast info via RPCs, for more informative queries and testing (#18038 comment)
add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (#18038 comment)

fanquake added the label P2P on May 6, 2020

fanquake commented at 0:42 am on May 6, 2020: member

My assumption is that those TODOs would be taken care of as part of #18807. Is that correct @amitiuttarwar ?

amitiuttarwar commented at 2:02 am on May 6, 2020: contributor

@fanquake sorry for the confusion 😛 @gzhao408 offered to implement these follow ups, so I asked her to open a PR so I & others could review. I was thinking (now that this PR is open), I’d update #18807 to let reviewers know, & focus that PR on updates to release notes and tests. does that seem reasonable?

fanquake commented at 2:06 am on May 6, 2020: member

does that seem reasonable? @amitiuttarwar Yea of course. Just wanted to clarify that you weren’t both working on the same changes in parallel.

glozow marked this as ready for review on May 6, 2020

glozow renamed this:
~~[wip] unbroadcast followups~~
unbroadcast followups
on May 6, 2020

amitiuttarwar commented at 2:11 am on May 6, 2020: contributor

@fanquake thanks!

Concept ACK :) will review soon

glozow force-pushed on May 6, 2020

in src/net_processing.cpp:819 in 7abdadf113 outdated

814@@ -815,7 +815,12 @@ void PeerLogicValidation::ReattemptInitialBroadcast(CScheduler& scheduler) const
815     std::set<uint256> unbroadcast_txids = m_mempool.GetUnbroadcastTxs();
816 
817     for (const uint256& txid : unbroadcast_txids) {
818-        RelayTransaction(txid, *connman);
819+        // Sanity check: all unbroadcast txns should exist in the mempool
820+        if (!m_mempool.exists(txid)) {

MarcoFalke commented at 4:02 pm on May 6, 2020:

0        if (m_mempool.exists(txid)) {

style-nit: (feel free to ignore)

When an if has an else branch, I generally recommend not using inversion in the condition, just to avoid double negation, which can make code harder to read. Obviously not an issue here, so feel free to ignore.

However, while this fights the symptom (unbroadcast txid exists), it doesn’t prevent it from happening. Also, RelayTransaction is hopefully already a noop if the txid does not exist.

What do you think about adding this check to the mempool member function that adds the unbroadcast transactions to the set? This way, the error will be caught as early as possible.

glozow commented at 6:37 pm on May 6, 2020:

Good point about inversion on an if+else, I’ll rearrange to make it more clear.

Yeah, I think it’d make sense to add this check to AddUnbroadcastTx (here). Apart from making sure we only add txids that exist in the mempool to unbroadcast, I think this sanity check helps if a tx was removed from the mempool but somehow stayed in the unbroadcast set - which shouldn’t happen, but just in case?

MarcoFalke commented at 6:47 pm on May 6, 2020:

Jup, better to do the check in two places than in one. This needs a mempool lookup, but the cost doesn’t seem too much, as this is only done every 10 minutes or when a new transaction is added to the mempool anyway.

in src/rpc/blockchain.cpp:1394 in 7abdadf113 outdated

1390@@ -1389,6 +1391,7 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
1391     ret.pushKV("maxmempool", (int64_t) maxmempool);
1392     ret.pushKV("mempoolminfee", ValueFromAmount(std::max(pool.GetMinFee(maxmempool), ::minRelayTxFee).GetFeePerK()));
1393     ret.pushKV("minrelaytxfee", ValueFromAmount(::minRelayTxFee.GetFeePerK()));
1394+    ret.pushKV("unbroadcastsize", (int64_t)pool.GetUnbroadcastTxs().size());

MarcoFalke commented at 4:04 pm on May 6, 2020:

style-nit: instead of a “blind cast” I slightly prefer the one that enables compile time checks and fails compilation in case it should be impossible to cast without loss of precision.

0    ret.pushKV("unbroadcastsize", int64_t{pool.GetUnbroadcastTxs().size())};

glozow commented at 6:31 pm on May 9, 2020:

That makes sense. The only issue is it wouldn’t be consistent with all the other casts throughout the file. I’m not sure what’s the best practice: let this one be inconsistent, change all of them in this PR, or change all of them in a followup PR?

MarcoFalke commented at 6:41 pm on May 9, 2020:

The guideline is that new code should follow best practices, old code can stay as is.

amitiuttarwar commented at 6:44 pm on May 9, 2020:

it’s tough to aim for consistency in bitcoin bc the project values careful review for any changes (thus refactor PRs are generally only considered if there’s a feature being built on top) so generally, we try to improve over time as we touch lines of code.

another example you’ll see is mismatched naming conventions. code that’s been touched recently adheres to the best practices, but code that’s been around for longer still follows old conventions

(tldr- improve what you touch over time instead of aiming for consistency)

glozow commented at 7:12 pm on May 9, 2020:

Makes sense, thank you!

MarcoFalke approved

MarcoFalke commented at 4:07 pm on May 6, 2020: member

Concept ACK, but it looks like the mempool_packages has a hardcoded mempool entry in the test, so that fails because the hardcoded entry is missing the unbraodcast key.

MarcoFalke added this to the milestone 0.21.0 on May 6, 2020

mzumsande commented at 10:24 pm on May 6, 2020: member

Concept ACK.

I think that mempool_packages.py just needs to wait a bit so that the unbroadcast status of txns does not change in between the getrawmempool and getmempoolentry calls because of delays in the p2p.

Also, it’s confusing that #18807 has (almost) the same name as this PR, so maybe change the title?

naumenkogs commented at 1:21 pm on May 7, 2020: member

Concept ACK. Code looks good, will do ACK when @MarcoFalke comments are addressed.

glozow renamed this:
~~unbroadcast followups~~
p2p: unbroadcast followups: rpcs, nLastResend, mempool sanity check
on May 7, 2020

glozow force-pushed on May 9, 2020

glozow commented at 10:04 pm on May 9, 2020: member

Sorry for all the force pushes 😅. Addressed MarcoFalke’s comments and the mempool_packages.py error. As mzumsande said, it was due to the unbroadcast value changing in between calls (i.e. a transaction completed initial broadcast while the test was running, so a subsequent call with verbose=True was inconsistent with before.

With unbroadcast as one of the mempool entry fields, it’s probably not a good idea to compare mempool entries as a whole, ie assert_equal(mempool0[tx], mempool1[tx] because unbroadcast might be different. Most tests are more specific than this, so they are ok. I fixed mempool_packages.py by having the node wait for len(mempool) invs, so all unbroadcasts should be False.

glozow force-pushed on May 9, 2020

mzumsande commented at 0:03 am on May 13, 2020: member

I meant an earlier spot in my comment: The assert_equal(entry, mempool[x]) in L92 can fail because mempool = self.nodes[0].getrawmempool(True) and entry = self.nodes[0].getmempoolentry(x) can have inconsistent unbroadcast flags if we haven’t waited before calling getrawmempool the first time.

glozow force-pushed on May 13, 2020

glozow commented at 0:40 am on May 13, 2020: member

I meant an earlier spot in my comment: The assert_equal(entry, mempool[x]) in L92 @mzumsande OMG I am so sorry 🤦‍♀️ that should have been obvious. Just fixed it (I hope), waiting to see what travis says.

DrahtBot commented at 8:38 am on May 13, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#12677 (RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in test/functional/mempool_packages.py:77 in 4ebe10fb9c outdated

73@@ -72,6 +74,10 @@ def run_test(self):
74             value = sent_value
75             chain.append(txid)
76 
77+        # Wait until mempool transactions have passed initial broadcast (got an inv for each one)

MarcoFalke commented at 12:59 pm on May 13, 2020:

The condition for initial broadcast is not that an inv was sent, but that a getdata was received

I think you can simply replace this with a self.sync_mempools() and remove the newly added p2p connection

glozow commented at 8:02 pm on May 13, 2020:

I think you can simply replace this with a self.sync_mempools() and remove the newly added p2p connection

I’ve tried this, but it times out: AssertionError: Mempool sync timed out after 60s

The condition for initial broadcast is not that an inv was sent, but that a getdata was received

Oh this is my misunderstanding. I can try to have a wait_until that counts the msg_getdatas received by nodes[0]? Open to suggestions

MarcoFalke commented at 8:07 pm on May 13, 2020:

Oh, I missed that the second node has a stricter limitancestorcount set.

In that case your suggestion to wait for the getdata makes most sense, I guess.

So

register callback in mininode to send getdata when inv arrives
wait for invs
sync_with_ping (to flush all buffers)

should work

glozow commented at 8:10 pm on May 13, 2020:

Thank you, this is super helpful! 🙏

in src/rpc/blockchain.cpp:1394 in 4ebe10fb9c outdated

1390@@ -1389,7 +1391,7 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
1391     ret.pushKV("maxmempool", (int64_t) maxmempool);
1392     ret.pushKV("mempoolminfee", ValueFromAmount(std::max(pool.GetMinFee(maxmempool), ::minRelayTxFee).GetFeePerK()));
1393     ret.pushKV("minrelaytxfee", ValueFromAmount(::minRelayTxFee.GetFeePerK()));
1394-
1395+    ret.pushKV("unbroadcastsize", static_cast<int64_t>(pool.GetUnbroadcastTxs().size()));

MarcoFalke commented at 1:03 pm on May 13, 2020:

0    ret.pushKV("unbroadcastsize", int64_t{pool.GetUnbroadcastTxs().size())};

This does the same thing, but has additional compile time checks that fail compilation when the conversion comes with a loss of precision.

glozow commented at 8:08 pm on May 13, 2020:

Really sorry about this - I got a compilation error:

0rpc/blockchain.cpp:1394:43: error: non-constant-expression cannot be narrowed from type 'std::__1::set<uint256, std::__1::less<uint256>,
1      std::__1::allocator<uint256> >::size_type' (aka 'unsigned long') to 'int64_t' (aka 'long long') in initializer list [-Wc++11-narrowing]
2    ret.pushKV("unbroadcastsize", int64_t{pool.GetUnbroadcastTxs().size()});
3                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4rpc/blockchain.cpp:1394:43: note: insert an explicit cast to silence this issue
5    ret.pushKV("unbroadcastsize", int64_t{pool.GetUnbroadcastTxs().size()});
6                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
7                                          static_cast<int64_t>(          )

So I followed the suggestion to fix it. I’m not sure why it didn’t work for me.

MarcoFalke commented at 8:21 pm on May 13, 2020:

Oh, I guess you can use this signature bool pushKV(const std::string&, uint64_t). It should already be selected by default, so I don’t see why we want to cast in the first place.

I think you can either:

Remove the cast
Select the signature for uint64_t

glozow force-pushed on May 14, 2020

in test/functional/mempool_packages.py:62 in c8ac4373ee outdated

58@@ -58,6 +59,7 @@ def chain_transaction(self, node, parent_txid, vout, value, fee, num_outputs):
59 
60     def run_test(self):
61         # Mine some blocks and have them mature.
62+        self.nodes[0].add_p2p_connection(P2PTxInvStore()) # keep track of invs

MarcoFalke commented at 11:56 pm on May 14, 2020:

You still need to provide the callback. The default one does not reply with a getdata. See

 0class P2PTxInvStore(P2PInterface):
 1    """A P2PInterface which stores a count of how many times each txid has been announced."""
 2    def __init__(self):
 3        super().__init__()
 4        self.tx_invs_received = defaultdict(int)
 5
 6    def on_inv(self, message):
 7        # Store how many times invs have been received for each tx.
 8        for i in message.inv:
 9            if i.type == MSG_TX:
10                # save txid
11                self.tx_invs_received[i.hash] += 1

MarcoFalke commented at 11:57 pm on May 14, 2020:

You can run git grep -W 'def on_inv' to find examples on how to do that.

MarcoFalke commented at 0:36 am on May 15, 2020:

Hmm. Looks like the tests passed on travis. I am still unsure why.

glozow commented at 1:12 am on May 15, 2020:

Gah, I made a super dumb assumption that it would automatically call P2PInterface’s on_inv (which responds with the getdata) since it inherits from it. I’m working on that right now. Not sure why travis passed haha

MarcoFalke commented at 12:19 pm on May 15, 2020:

I reset travis, so that it properly shows failure. :sweat_smile:

glozow commented at 11:48 pm on May 15, 2020:

I spoke with @amitiuttarwar and decided to just have P2PTxInvStore call super().on_inv since she’s planning on doing it anyway (in #18807). We’ve agreed to resolve any conflicts that happen depending on which gets merged first.

glozow force-pushed on May 15, 2020

in test/functional/test_framework/mininode.py:655 in bc2d7a9f25 outdated

647@@ -647,3 +648,11 @@ def on_inv(self, message):
648     def get_invs(self):
649         with mininode_lock:
650             return list(self.tx_invs_received.keys())
651+
652+    def wait_for_broadcast(self, num_txns):
653+        # Waits for num_txns transactions to complete initial broadcast.
654+        # Wait until num_txns invs have been received (and getdatas sent).
655+        wait_until(lambda: len(self.tx_invs_received.keys()) == num_txns)

MarcoFalke commented at 4:26 pm on May 16, 2020:

0        self.wait_until(lambda: set(self.get_invs()) == set(txns))

self.wait_until because we need the scaling factor
self.get_inv because we need to take the lock
set because we want to check the txids

glozow commented at 6:43 pm on May 17, 2020:

This makes sense.

It seems like a P2PInterfaces don’t have a self.wait_until function. I get a object has no attribute 'wait_until' error. The classes in mininode.py use wait_until with the mininode_lock. I’m not familiar with what you mean by scaling factor, but is there something else I could do?

MarcoFalke commented at 7:33 pm on May 17, 2020:

Are you sure you are on current master? I am seeing this function locally:

0$ git grep 'def wait_until'
1test/functional/test_framework/mininode.py:    def wait_until(self, test_function, timeout):

MarcoFalke approved

MarcoFalke commented at 4:26 pm on May 16, 2020: member

Almost ACK

[wallet] remove nLastResend logic

remove nLastResend because it's unnecessary now that rebroadcasts always happen at least 12 hours later

d160069604

glozow force-pushed on May 18, 2020

in src/txmempool.h:723 in 827fc85807 outdated

718@@ -716,6 +719,12 @@ class CTxMemPool
719         return m_unbroadcast_txids;
720     }
721 
722+    /** Returns if a txid is in the unbroadcast set */
723+    const bool InUnbroadcast(const uint256& txid) const {

amitiuttarwar commented at 1:09 am on May 19, 2020:

when compiling, I’m seeing warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]

just took a little dive down the c++ rabbit hole.. I think declaring the return type as a const is meaningless if you are returning a built-in type by value. so in this example, the const wouldn’t be enforcing anything on the caller.

amitiuttarwar commented at 1:56 am on May 19, 2020:

something to consider- to me, the naming IsUnbroadcast feels more natural. A transaction is in the mempool, but it might also be in the unbroadcast state.

but InUnbroadcast is still accurate in terms of implementation, since they are stored in the set of m_unbroadcast_txids.

so might just be a preference thing. please feel free not to update :)

glozow commented at 8:24 pm on May 19, 2020:

Ok after over-thinking this for a few hours 😂 I’m going to change it to IsUnbroadcastTx which looks pretty similar to the others and hopefully feels natural. I definitely think entry.IsUnbroadcast() would be ideal if it were a mempool entry method (I didn’t do it that way because m_unbroadcast_txids belongs to the mempool). I just don’t want it to sound like I’m saying the mempool is unbroadcast.

in test/functional/mempool_packages.py:149 in 827fc85807 outdated

145@@ -140,6 +146,7 @@ def run_test(self):
146                     assert_equal(ainfo['depends'], [])
147 
148 
149+

amitiuttarwar commented at 1:22 am on May 19, 2020:

accidental extra whitespace?

glozow commented at 6:58 pm on May 19, 2020:

😅 I’ll delete this

in src/rpc/blockchain.cpp:402 in 827fc85807 outdated

398@@ -399,6 +399,7 @@ static std::vector<RPCResult> MempoolEntryDescription() { return {
399     RPCResult{RPCResult::Type::ARR, "spentby", "unconfirmed transactions spending outputs from this transaction",
400         {RPCResult{RPCResult::Type::STR_HEX, "transactionid", "child transaction id"}}},
401     RPCResult{RPCResult::Type::BOOL, "bip125-replaceable", "Whether this transaction could be replaced due to BIP125 (replace-by-fee)"},
402+    RPCResult{RPCResult::Type::BOOL, "unbroadcast", "Whether this transaction is currently unbroadcast (not yet past initial broadcast)"},

amitiuttarwar commented at 1:27 am on May 19, 2020:

consider updating part in parentheses to: (initial broadcast not yet confirmed)

its a bit more accurate, but hard for me to evaluate clarity for end users 😛

in src/rpc/blockchain.cpp:1413 in 827fc85807 outdated

1409@@ -1408,6 +1410,7 @@ static UniValue getmempoolinfo(const JSONRPCRequest& request)
1410                         {RPCResult::Type::NUM, "maxmempool", "Maximum memory usage for the mempool"},
1411                         {RPCResult::Type::STR_AMOUNT, "mempoolminfee", "Minimum fee rate in " + CURRENCY_UNIT + "/kB for tx to be accepted. Is the maximum of minrelaytxfee and minimum mempool fee"},
1412                         {RPCResult::Type::STR_AMOUNT, "minrelaytxfee", "Current minimum relay fee for transactions"},
1413+                        {RPCResult::Type::NUM, "unbroadcastsize", "Current number of transactions that haven't passed initial broadcast yet"}

amitiuttarwar commented at 1:32 am on May 19, 2020:

same as above, consider updating description.

also unbroadcastcount might make more sense than unbroadcastsize?

in src/txmempool.h:707 in 827fc85807 outdated

703@@ -704,7 +704,10 @@ class CTxMemPool
704     /** Adds a transaction to the unbroadcast set */
705     void AddUnbroadcastTx(const uint256& txid) {
706         LOCK(cs);
707-        m_unbroadcast_txids.insert(txid);
708+        /** Sanity Check: the transaction should also be in the mempool */

amitiuttarwar commented at 5:03 am on May 19, 2020:

hm, is there a reason this is a doxygen comment? I’m not sure what would happen, but I suspect you want normal // notation here

jonatack commented at 7:45 am on May 22, 2020:

I suspect you want normal // notation here

Agree, perhaps can be touched up in #18807

in test/functional/test_framework/mininode.py:666 in 827fc85807 outdated

660+        # Waits for the txns (txids) to complete initial broadcast.
661+        # Wait until invs have been received (and getdatas sent) for each txid.
662+        self.wait_until(lambda: set(self.get_invs()) == set([int(tx, 16) for tx in txns]), 60)
663+        # Flush messages and wait for the getdatas to be processed
664+        # The mempool should mark unbroadcast=False for these transactions.
665+        self.sync_with_ping()

amitiuttarwar commented at 5:31 am on May 19, 2020:

nit: pull out the description of the function from the implementation & description for readability

0    # Waits for the txns (txids) to complete initial broadcast.
1    # The mempool should mark unbroadcast=False for these transactions.
2    def wait_for_broadcast(self, txns):
3        # Wait until invs have been received (and getdatas sent) for each txid.
4        self.wait_until(lambda: set(self.get_invs()) == set([int(tx, 16) for tx in txns]), 60)
5        # Flush messages and wait for the getdatas to be processed
6        self.sync_with_ping()

glozow commented at 9:26 pm on May 19, 2020:

Good point, gonna follow this PEP-8 guideline

amitiuttarwar commented at 5:50 am on May 19, 2020: contributor

overall, the code looks good! the approaches are simple & make sense. solid test coverage. I also appreciate the thoughtful documentation :) thank you for taking this on!

I left a bunch of nits and thoughts to consider. Other than removing the const to get rid of the build warning, feel free to take or leave whatever.

Just to note- this code adds unbroadcast info to 5 different RPC methods. Of these 5, the unbroadcast info of 2 are explicitly tested from the functional tests (getmempoolinfo, getrawmempool). But the others are implicitly tested because its a shared data structure that serves the data (getmempoolancestors, getmempooldescendants, getmempoolentry). That coverage seems sufficient to me.

I like the wait_for_broadcast method & how it encapsulates the synchronization required to ensure a txn is removed from the unbroadcast set. have you checked if there are other rpc call sites in the tests that might benefit (aka help prevent weird race conditions)? I’ll double check this for my next round of review.

thanks!

amitiuttarwar commented at 6:23 pm on May 19, 2020: contributor

also I realized, since this updates some RPC endpoints, should there be release notes added? @MarcoFalke

MarcoFalke commented at 6:26 pm on May 19, 2020: member

Sure, but I assumed this would be taken care of in the doc pr #18807

[rpc] add unbroadcast info to mempool entries and getmempoolinfo

- expose info about number of txns in unbroadcast set and whether a mempool entry's tx has passed initial broadcast
- makes rpcs more informative and allows for more explicit testing, eg tracking if tx is in unbroadcast set
before and after originating node connects to peers (adds this in mempool_unbroadcast.py)
- adds mempool method IsUnbroadcastTx to query for tx inclusion in  mempool's unbroadcast set

a7ebe48b94

[mempool] sanity check that all unbroadcast txns are in mempool

- before reattempting broadcast for unbroadcast txns, check they are in mempool and remove if not
- this protects from memory leaks and network spam just in case unbroadcast set (incorrectly) has extra txns
- check that tx is in mempool before adding to unbroadcast set to try to prevent this from happening

9d3f7eb986

[test] wait for inital broadcast before comparing mempool entries

- mempool entry 'unbroadcast' field changes when tx passes initial broadcast (receive getdata),
so anytime you compare mempool entries as a whole, you must wait for all broadcasts to complete
('unbroadcast' = False) otherwise the state may change in between calls
- update P2PTxInvStore to send msg_getdata for invs and add functionality to wait for a list
of txids to complete initial broadcast
- make mempool_packages.py wait because it compares entries using getrawmempool and
getmempoolentry

651f1d816f

in test/functional/test_framework/mininode.py:662 in 827fc85807 outdated

654@@ -654,3 +655,11 @@ def on_inv(self, message):
655     def get_invs(self):
656         with mininode_lock:
657             return list(self.tx_invs_received.keys())
658+
659+    def wait_for_broadcast(self, txns):
660+        # Waits for the txns (txids) to complete initial broadcast.
661+        # Wait until invs have been received (and getdatas sent) for each txid.
662+        self.wait_until(lambda: set(self.get_invs()) == set([int(tx, 16) for tx in txns]), 60)

MarcoFalke commented at 7:22 pm on May 19, 2020:

0        self.wait_until(lambda: set(self.get_invs()) == set([int(tx, 16) for tx in txns]), timeout)

I think it makes sense to define a default value of timeout=60 for the wait_for_broadcast member function. This way, callers can decide to overwrite it if needed.

glozow force-pushed on May 19, 2020

MarcoFalke commented at 0:07 am on May 20, 2020: member

Review ACK 651f1d816f054cb9c637f8a99c9360bba381ef58

fanquake requested review from naumenkogs on May 20, 2020

fanquake requested review from ajtowns on May 20, 2020

naumenkogs approved

naumenkogs commented at 11:40 pm on May 20, 2020: member

Code review ACK 651f1d816f054cb9c637f8a99c9360bba381ef58

fanquake requested review from amitiuttarwar on May 21, 2020

amitiuttarwar commented at 7:27 pm on May 21, 2020: contributor

ACK 651f1d816f054cb9c637f8a99c9360bba381ef58 🎉

double checked functional tests for possible races around returning unbroadcast from the various RPCs. looks like mempool_packages is the only one comparing all the fields of the returned object, so your changes have them covered.

fanquake merged this on May 21, 2020

fanquake closed this on May 21, 2020

sidhujag referenced this in commit 2c5b7d1147 on May 22, 2020

in src/txmempool.h:722 in 651f1d816f

718@@ -716,6 +719,12 @@ class CTxMemPool
719         return m_unbroadcast_txids;
720     }
721 
722+    // Returns if a txid is in the unbroadcast set

jonatack commented at 7:47 am on May 22, 2020:

This can be doxygen like the neighbouring functions. Perhaps touch up in #18807.

amitiuttarwar commented at 8:07 pm on May 23, 2020:

done

jonatack commented at 7:48 am on May 22, 2020: member

Posthumous code review ACK 651f1d816f054cb9c637f8a99c9360bba381ef58

in src/rpc/blockchain.cpp:402 in 651f1d816f

398@@ -399,6 +399,7 @@ static std::vector<RPCResult> MempoolEntryDescription() { return {
399     RPCResult{RPCResult::Type::ARR, "spentby", "unconfirmed transactions spending outputs from this transaction",
400         {RPCResult{RPCResult::Type::STR_HEX, "transactionid", "child transaction id"}}},
401     RPCResult{RPCResult::Type::BOOL, "bip125-replaceable", "Whether this transaction could be replaced due to BIP125 (replace-by-fee)"},
402+    RPCResult{RPCResult::Type::BOOL, "unbroadcast", "Whether this transaction is currently unbroadcast (initial broadcast not yet confirmed)"},

harding commented at 10:52 am on May 23, 2020:

Suggested rephrasing: s/confirmed/acknowledged/. I think “confirmed” might confuse the user into thinking we’re referring to the transaction being included in a block. Perhaps “acknowledged” would be less confusing, or if a few more words are ok, “(initial broadcast not yet acknowledged by any peers)”. Maybe @amitiuttarwar can squeeze this in #18807 (if not, I can open a separate PR).

amitiuttarwar commented at 5:56 pm on May 23, 2020:

great suggestion. adding to #18807

amitiuttarwar commented at 8:07 pm on May 23, 2020:

done

glozow deleted the branch on May 25, 2020

MarcoFalke referenced this in commit 826fe9c667 on May 30, 2020

sidhujag referenced this in commit e89c4bd9d2 on May 31, 2020

ComputerCraftr referenced this in commit 6d07be106f on Jun 10, 2020

ComputerCraftr referenced this in commit d2fddf6f43 on Jun 10, 2020

ComputerCraftr referenced this in commit 4abc92194d on Jun 10, 2020

ComputerCraftr referenced this in commit 1cf6ab1d17 on Jun 10, 2020

Fabcien referenced this in commit 2666512cfb on Jan 22, 2021

Fabcien referenced this in commit e275c3dc80 on Jan 22, 2021

Fabcien referenced this in commit 002b33b419 on Jan 22, 2021

MarcoFalke locked this on Feb 15, 2022

p2p: unbroadcast followups: rpcs, nLastResend, mempool sanity check #18895

Conflicts