Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions src/addrman.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -846,6 +846,30 @@ std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct
return addresses;
}

std::vector<std::pair<AddrInfo, AddressPosition>> AddrManImpl::GetEntries_(bool from_tried) const
{
AssertLockHeld(cs);

const int bucket_count = from_tried ? ADDRMAN_TRIED_BUCKET_COUNT : ADDRMAN_NEW_BUCKET_COUNT;
std::vector<std::pair<AddrInfo, AddressPosition>> infos;
for (int bucket = 0; bucket < bucket_count; ++bucket) {
for (int position = 0; position < ADDRMAN_BUCKET_SIZE; ++position) {
int id = GetEntry(from_tried, bucket, position);
if (id >= 0) {
AddrInfo info = mapInfo.at(id);
AddressPosition location = AddressPosition(
from_tried,
/*multiplicity_in=*/from_tried ? 1 : info.nRefCount,
bucket,
position);
infos.push_back(std::make_pair(info, location));
}
}
}

return infos;
}

void AddrManImpl::Connected_(const CService& addr, NodeSeconds time)
{
AssertLockHeld(cs);
Expand Down Expand Up @@ -1218,6 +1242,15 @@ std::vector<CAddress> AddrManImpl::GetAddr(size_t max_addresses, size_t max_pct,
return addresses;
}

std::vector<std::pair<AddrInfo, AddressPosition>> AddrManImpl::GetEntries(bool from_tried) const
{
LOCK(cs);
Check();
auto addrInfos = GetEntries_(from_tried);
Check();
return addrInfos;
}

void AddrManImpl::Connected(const CService& addr, NodeSeconds time)
{
LOCK(cs);
Expand Down Expand Up @@ -1320,6 +1353,11 @@ std::vector<CAddress> AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std
return m_impl->GetAddr(max_addresses, max_pct, network, filtered);
}

std::vector<std::pair<AddrInfo, AddressPosition>> AddrMan::GetEntries(bool use_tried) const
{
return m_impl->GetEntries(use_tried);
}

void AddrMan::Connected(const CService& addr, NodeSeconds time)
{
m_impl->Connected(addr, time);
Expand Down
13 changes: 12 additions & 1 deletion src/addrman.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class AddrManImpl;
/** Default for -checkaddrman */
static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0};

/** Test-only struct, capturing info about an address in AddrMan */
/** Location information for an address in AddrMan */
struct AddressPosition {
// Whether the address is in the new or tried table
const bool tried;
Expand Down Expand Up @@ -183,6 +183,17 @@ class AddrMan
*/
std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const;

/**
* Returns an information-location pair for all addresses in the selected addrman table.
* If an address appears multiple times in the new table, an information-location pair
* is returned for each occurence. Addresses only ever appear once in the tried table.
*
* @param[in] from_tried Selects which table to return entries from.
*
* @return A vector consisting of pairs of AddrInfo and AddressPosition.
*/
std::vector<std::pair<AddrInfo, AddressPosition>> GetEntries(bool from_tried) const;

/** We have successfully connected to this peer. Calling this function
* updates the CAddress's nTime, which is used in our IsTerrible()
* decisions and gossiped to peers. Callers should be careful that updating
Expand Down
5 changes: 5 additions & 0 deletions src/addrman_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,9 @@ class AddrManImpl
std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const
EXCLUSIVE_LOCKS_REQUIRED(!cs);

std::vector<std::pair<AddrInfo, AddressPosition>> GetEntries(bool from_tried) const
EXCLUSIVE_LOCKS_REQUIRED(!cs);

void Connected(const CService& addr, NodeSeconds time)
EXCLUSIVE_LOCKS_REQUIRED(!cs);

Expand Down Expand Up @@ -264,6 +267,8 @@ class AddrManImpl

std::vector<CAddress> GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const EXCLUSIVE_LOCKS_REQUIRED(cs);

std::vector<std::pair<AddrInfo, AddressPosition>> GetEntries_(bool from_tried) const EXCLUSIVE_LOCKS_REQUIRED(cs);

void Connected_(const CService& addr, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs);

void SetServices_(const CService& addr, ServiceFlags nServices) EXCLUSIVE_LOCKS_REQUIRED(cs);
Expand Down
4 changes: 2 additions & 2 deletions src/rpc/client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -458,10 +458,10 @@ UniValue RPCConvertNamedValues(std::string strMethod, const std::vector<std::str
}

if (!positional_args.empty()) {
// Use __pushKV instead of pushKV to avoid overwriting an explicit
// Use pushKVEnd instead of pushKV to avoid overwriting an explicit
// "args" value with an implicit one. Let the RPC server handle the
// request as given.
params.__pushKV("args", positional_args);
params.pushKVEnd("args", positional_args);
}

return params;
Expand Down
4 changes: 2 additions & 2 deletions src/rpc/mempool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -346,8 +346,8 @@ UniValue MempoolToJSON(const CTxMemPool& pool, const llmq::CInstantSendManager*
entryToJSON(pool, info, e, isman);
// Mempool has unique entries so there is no advantage in using
// UniValue::pushKV, which checks if the key already exists in O(N).
// UniValue::__pushKV is used instead which currently is O(1).
o.__pushKV(hash.ToString(), info);
// UniValue::pushKVEnd is used instead which currently is O(1).
o.pushKVEnd(hash.ToString(), info);
}
return o;
} else {
Expand Down
70 changes: 70 additions & 0 deletions src/rpc/net.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <rpc/server.h>

#include <addrman.h>
#include <addrman_impl.h>
#include <banman.h>
#include <chainparams.h>
#include <clientversion.h>
Expand Down Expand Up @@ -1174,6 +1175,74 @@ static RPCHelpMan getaddrmaninfo()
};
}

UniValue AddrmanEntryToJSON(const AddrInfo& info)
{
UniValue ret(UniValue::VOBJ);
ret.pushKV("address", info.ToStringAddr());
ret.pushKV("port", info.GetPort());
ret.pushKV("services", (uint64_t)info.nServices);
ret.pushKV("time", int64_t{TicksSinceEpoch<std::chrono::seconds>(info.nTime)});
ret.pushKV("network", GetNetworkName(info.GetNetClass()));
ret.pushKV("source", info.source.ToStringAddr());
ret.pushKV("source_network", GetNetworkName(info.source.GetNetClass()));
return ret;
}

UniValue AddrmanTableToJSON(const std::vector<std::pair<AddrInfo, AddressPosition>>& tableInfos)
{
UniValue table(UniValue::VOBJ);
for (const auto& e : tableInfos) {
AddrInfo info = e.first;
AddressPosition location = e.second;
std::ostringstream key;
key << location.bucket << "/" << location.position;
// Address manager tables have unique entries so there is no advantage
// in using UniValue::pushKV, which checks if the key already exists
// in O(N). UniValue::pushKVEnd is used instead which currently is O(1).
table.pushKVEnd(key.str(), AddrmanEntryToJSON(info));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Reorder cherry-picks: bitcoin#28523 uses pushKVEnd before bitcoin#27822 introduces it (bisect hazard)

The series orders the merges as a5264f9 (bitcoin#28523) → c34e457 (bitcoin#27822) → 3c47eb7 (bitcoin#28671). bitcoin#28523's new AddrmanTableToJSON calls table.pushKVEnd(...) (src/rpc/net.cpp:1202), but UniValue::pushKVEnd is introduced only by bitcoin#27822, which is the next commit in this PR.

Verified: git show a5264f984a:src/univalue/include/univalue.h still declares only __pushKV, not pushKVEnd. Commit a5264f9 therefore does not compile in isolation, breaking git bisect across these two commits. The final HEAD is fine because 27822 is applied.

Upstream chronological order is bitcoin#27822 (2023-06-21) → bitcoin#28523 (2023-10-03). Please reorder the cherry-picks to match upstream so every intermediate commit builds. (If you also restore bitcoin#26638/bitcoin#28976 per the other finding, this is a good opportunity to fix the order while re-pushing.)

source: ['claude', 'codex']

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Commit ordering: 28523 uses pushKVEnd before 27822 introduces it (bisect hazard)

Within this PR the commits are ordered a5264f9 (Merge bitcoin#28523) → c34e457 (Merge bitcoin#27822) → 3c47eb7 (Merge bitcoin#28671). bitcoin#28523's new AddrmanTableToJSON calls table.pushKVEnd(...) (src/rpc/net.cpp:1202), but pushKVEnd is only introduced by bitcoin#27822, which is the next commit. Verified: git show a5264f984a:src/univalue/include/univalue.h shows only __pushKV, not pushKVEnd. Result: commit a5264f9 does not compile in isolation. The final tree at HEAD is fine (27822 is applied), and runtime behavior is unaffected, but this breaks git bisect between these two commits. Upstream chronological order is 27822 (2023-06-21) → 28523 (2023-10-03); please reorder the cherry-picks to match.


Policy gate (backport-prereq-restore): For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. intentional_exclusion: true or a matching entry in policy_overrides). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.

source: ['claude-backport-reviewer']

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Missing prerequisite before cherry-pick: bitcoin#27822

Dash commit a5264f9, the bitcoin#28523 backport, adds a call to table.pushKVEnd(...). In upstream, bitcoin#28523's parent already had UniValue::pushKVEnd because bitcoin#27822 was merged earlier. In Dash, a5264f9^ still declares only UniValue::__pushKV, and pushKVEnd is introduced later by c34e457. This means the dependency chain is present only in the final head, not at the cherry-pick point; the 28523 commit itself is unbuildable. The same commit also adds a test using time.time() before Dash adds the import in the later 28671 adaptation.


Policy gate (backport-prereq-restore): For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. intentional_exclusion: true or a matching entry in policy_overrides). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.

source: ['codex-backport-reviewer']

}
return table;
}

static RPCHelpMan getrawaddrman()
{
return RPCHelpMan{"getrawaddrman",
"EXPERIMENTAL warning: this call may be changed in future releases.\n"
"\nReturns information on all address manager entries for the new and tried tables.\n",
{},
RPCResult{
RPCResult::Type::OBJ_DYN, "", "", {
{RPCResult::Type::OBJ_DYN, "table", "buckets with addresses in the address manager table ( new, tried )", {
{RPCResult::Type::OBJ, "bucket/position", "the location in the address manager table (<bucket>/<position>)", {
{RPCResult::Type::STR, "address", "The address of the node"},
{RPCResult::Type::NUM, "port", "The port number of the node"},
{RPCResult::Type::STR, "network", "The network (" + Join(GetNetworkNames(), ", ") + ") of the address"},
{RPCResult::Type::NUM, "services", "The services offered by the node"},
{RPCResult::Type::NUM_TIME, "time", "The " + UNIX_EPOCH_TIME + " when the node was last seen"},
{RPCResult::Type::STR, "source", "The address that relayed the address to us"},
{RPCResult::Type::STR, "source_network", "The network (" + Join(GetNetworkNames(), ", ") + ") of the source address"},
}}
}}
}
},
RPCExamples{
HelpExampleCli("getrawaddrman", "")
+ HelpExampleRpc("getrawaddrman", "")
},
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {
NodeContext& node = EnsureAnyNodeContext(request.context);
if (!node.addrman) {
throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Address manager functionality missing or disabled");
}

UniValue ret(UniValue::VOBJ);
ret.pushKV("new", AddrmanTableToJSON(node.addrman->GetEntries(false)));
ret.pushKV("tried", AddrmanTableToJSON(node.addrman->GetEntries(true)));
return ret;
},
};
}

void RegisterNetRPCCommands(CRPCTable &t)
{
static const CRPCCommand commands[]{
Expand All @@ -1196,6 +1265,7 @@ void RegisterNetRPCCommands(CRPCTable &t)
{"hidden", &sendmsgtopeer},
{"hidden", &setmnthreadactive},
{"hidden", &getaddrmaninfo},
{"hidden", &getrawaddrman},
};
for (const auto& c : commands) {
t.appendCommand(c.name, &c);
Expand Down
1 change: 1 addition & 0 deletions src/test/fuzz/rpc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ const std::vector<std::string> RPC_COMMANDS_SAFE_FOR_FUZZING{
"getnetworkinfo",
"getnodeaddresses",
"getpeerinfo",
"getrawaddrman",
"getrawmempool",
"getrawtransaction",
"getrpcinfo",
Expand Down
2 changes: 1 addition & 1 deletion src/test/settings_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ inline std::ostream& operator<<(std::ostream& os, const util::SettingsValue& val
inline std::ostream& operator<<(std::ostream& os, const std::pair<std::string, util::SettingsValue>& kv)
{
util::SettingsValue out(util::SettingsValue::VOBJ);
out.__pushKV(kv.first, kv.second);
out.pushKVEnd(kv.first, kv.second);
os << out.write();
return os;
}
Expand Down
12 changes: 6 additions & 6 deletions src/txorphanage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,10 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer)
int TxOrphanage::EraseTx(const uint256& txid)
{
LOCK(m_mutex);
return _EraseTx(txid);
return EraseTxNoLock(txid);
}

int TxOrphanage::_EraseTx(const uint256& txid)
int TxOrphanage::EraseTxNoLock(const uint256& txid)
{
AssertLockHeld(m_mutex);
std::map<uint256, OrphanTx>::iterator it = m_orphans.find(txid);
Expand Down Expand Up @@ -110,7 +110,7 @@ void TxOrphanage::EraseForPeer(NodeId peer)
std::map<uint256, OrphanTx>::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid
if (maybeErase->second.fromPeer == peer)
{
nErased += _EraseTx(maybeErase->second.tx->GetHash());
nErased += EraseTxNoLock(maybeErase->second.tx->GetHash());
}
}
if (nErased > 0) LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx from peer=%d\n", nErased, peer);
Expand All @@ -132,7 +132,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans_size)
{
std::map<uint256, OrphanTx>::iterator maybeErase = iter++;
if (maybeErase->second.nTimeExpire <= nNow) {
nErased += _EraseTx(maybeErase->second.tx->GetHash());
nErased += EraseTxNoLock(maybeErase->second.tx->GetHash());
} else {
nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime);
}
Expand All @@ -146,7 +146,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans_size)
{
// Evict a random orphan:
size_t randompos = rng.randrange(m_orphan_list.size());
_EraseTx(m_orphan_list[randompos]->first);
EraseTxNoLock(m_orphan_list[randompos]->first);
++nEvicted;
}
if (nEvicted > 0) LogPrint(BCLog::MEMPOOL, "orphanage overflow, removed %u tx\n", nEvicted);
Expand Down Expand Up @@ -234,7 +234,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block)
if (vOrphanErase.size()) {
int nErased = 0;
for (const uint256& orphanHash : vOrphanErase) {
nErased += _EraseTx(orphanHash);
nErased += EraseTxNoLock(orphanHash);
}
LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx included or conflicted by block\n", nErased);
}
Expand Down
2 changes: 1 addition & 1 deletion src/txorphanage.h
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ class TxOrphanage {
size_t m_orphan_tx_size GUARDED_BY(m_mutex){0};

/** Erase an orphan by txid */
int _EraseTx(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
int EraseTxNoLock(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
};

#endif // BITCOIN_TXORPHANAGE_H
2 changes: 1 addition & 1 deletion src/univalue/include/univalue.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ class UniValue {
template <class It>
void push_backV(It first, It last);

void __pushKV(std::string key, UniValue val);
void pushKVEnd(std::string key, UniValue val);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Do not modify vendored UniValue sources without an explicit exception.

Line 90 introduces an API rename inside src/univalue/**, which violates the repository rule forbidding edits to vendored dependencies. Please either (a) keep vendor code untouched and adapt callers externally, or (b) document/approve an explicit exception for this backport.

As per coding guidelines: src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue,crypto/{ctaes,x11}}/**: “Do not make changes to vendored dependencies…”.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/univalue/include/univalue.h` at line 90, The modification to
src/univalue/include/univalue.h violates the repository rule forbidding changes
to vendored dependencies. The pushKVEnd method addition at line 90 modifies
vendored UniValue code which must remain untouched. To fix this, either revert
the changes to src/univalue/** and instead adapt all external callers to work
with the original UniValue API, or obtain explicit documented approval from the
maintainers for an exception to the vendored code policy before proceeding with
this backport.

Source: Coding guidelines

void pushKV(std::string key, UniValue val);
void pushKVs(UniValue obj);

Expand Down
6 changes: 3 additions & 3 deletions src/univalue/lib/univalue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ void UniValue::push_backV(const std::vector<UniValue>& vec)
values.insert(values.end(), vec.begin(), vec.end());
}

void UniValue::__pushKV(std::string key, UniValue val)
void UniValue::pushKVEnd(std::string key, UniValue val)
{
checkType(VOBJ);

Expand All @@ -131,7 +131,7 @@ void UniValue::pushKV(std::string key, UniValue val)
if (findKey(key, idx))
values[idx] = std::move(val);
else
__pushKV(std::move(key), std::move(val));
pushKVEnd(std::move(key), std::move(val));
}

void UniValue::pushKVs(UniValue obj)
Expand All @@ -140,7 +140,7 @@ void UniValue::pushKVs(UniValue obj)
obj.checkType(VOBJ);

for (size_t i = 0; i < obj.keys.size(); i++)
__pushKV(std::move(obj.keys.at(i)), std::move(obj.values.at(i)));
pushKVEnd(std::move(obj.keys.at(i)), std::move(obj.values.at(i)));
}

void UniValue::getObjMap(std::map<std::string,UniValue>& kv) const
Expand Down
4 changes: 2 additions & 2 deletions src/univalue/test/object.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ void univalue_push_throw()
UniValue j;
BOOST_CHECK_THROW(j.push_back(1), std::runtime_error);
BOOST_CHECK_THROW(j.push_backV({1}), std::runtime_error);
BOOST_CHECK_THROW(j.__pushKV("k", 1), std::runtime_error);
BOOST_CHECK_THROW(j.pushKVEnd("k", 1), std::runtime_error);
BOOST_CHECK_THROW(j.pushKV("k", 1), std::runtime_error);
BOOST_CHECK_THROW(j.pushKVs({}), std::runtime_error);
}
Expand Down Expand Up @@ -364,7 +364,7 @@ void univalue_object()
obj.setObject();
UniValue uv;
uv.setInt(42);
obj.__pushKV("age", uv);
obj.pushKVEnd("age", uv);
BOOST_CHECK_EQUAL(obj.size(), 1);
BOOST_CHECK_EQUAL(obj["age"].getValStr(), "42");

Expand Down
2 changes: 1 addition & 1 deletion src/util/settings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ bool WriteSettings(const fs::path& path,
{
SettingsValue out(SettingsValue::VOBJ);
for (const auto& value : values) {
out.__pushKV(value.first, value.second);
out.pushKVEnd(value.first, value.second);
}
std::ofstream file;
file.open(path);
Expand Down
4 changes: 2 additions & 2 deletions src/wallet/rpc/addresses.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -608,11 +608,11 @@ RPCHelpMan getaddressesbylabel()
CHECK_NONFATAL(unique);
// UniValue::pushKV checks if the key exists in O(N)
// and since duplicate addresses are unexpected (checked with
// std::set in O(log(N))), UniValue::__pushKV is used instead,
// std::set in O(log(N))), UniValue::pushKVEnd is used instead,
// which currently is O(1).
UniValue value(UniValue::VOBJ);
value.pushKV("purpose", _purpose);
ret.__pushKV(address, value);
ret.pushKVEnd(address, value);
}
});

Expand Down
Loading
Loading