CAddrMan: stochastic address manager #787

Merged
merged 1 commit into from Mar 22, 2012

Conversation

Projects
None yet
3 participants
@sipa
Owner

sipa commented Jan 27, 2012

Design goals:

  • Only keep a limited number of addresses around, so that addr.dat does not grow without bound.
  • Keep the address tables in-memory, and occasionally write the table to addr.dat.
  • Make sure no (localized) attacker can fill the entire table with his nodes/addresses.

See comments in addrman.h for more detailed information.

@gavinandresen

This comment has been minimized.

Show comment Hide comment
@gavinandresen

gavinandresen Jan 27, 2012

Contributor

What needs testing with these changes?

I assume:

  • Deals with existing addr.dat
  • Is faster
  • Works with -noirc -nodnsseed and a removed addr.dat (bootstrap from seed nodes)

Do you have a tool for comparing two addrman databases? Might be nice to verify that two copies of bitcoin started at about the same time end up with mostly different peer-sets.

Contributor

gavinandresen commented Jan 27, 2012

What needs testing with these changes?

I assume:

  • Deals with existing addr.dat
  • Is faster
  • Works with -noirc -nodnsseed and a removed addr.dat (bootstrap from seed nodes)

Do you have a tool for comparing two addrman databases? Might be nice to verify that two copies of bitcoin started at about the same time end up with mostly different peer-sets.

@sipa

This comment has been minimized.

Show comment Hide comment
@sipa

sipa Jan 28, 2012

Owner

Some improvements and bugfixes done.

@gavinandresen:

  • It will convert an old addr.dat to the new format. During the first start-up, all addr.dat entries are imported. During a second start-up, all old addr entries are removed (using CDB::Rewrite).
  • It is faster in the sense that all locking issues involving CAddrDB and cs_mapAddresses are gone, and address-operations are fast. Reaching 8 connections may be a bit slower, as the new code much less strongly favors trying recently seen addresses - that is the price to pay for some protection against sybil attacks. When connection count is low, it will however favor self-tried addresses.
    • Yes, booting from seed nodes is no problem.

The main thing to be tested is whether the bucket count (256 and 64) and bucket size (64) are large enough, knowing that they do not get quickly filled completely. For example, a new-style addr.dat of a week old, will it easily find connections when ran using -noirc -nodnsseed -nolisten?

Owner

sipa commented Jan 28, 2012

Some improvements and bugfixes done.

@gavinandresen:

  • It will convert an old addr.dat to the new format. During the first start-up, all addr.dat entries are imported. During a second start-up, all old addr entries are removed (using CDB::Rewrite).
  • It is faster in the sense that all locking issues involving CAddrDB and cs_mapAddresses are gone, and address-operations are fast. Reaching 8 connections may be a bit slower, as the new code much less strongly favors trying recently seen addresses - that is the price to pay for some protection against sybil attacks. When connection count is low, it will however favor self-tried addresses.
    • Yes, booting from seed nodes is no problem.

The main thing to be tested is whether the bucket count (256 and 64) and bucket size (64) are large enough, knowing that they do not get quickly filled completely. For example, a new-style addr.dat of a week old, will it easily find connections when ran using -noirc -nodnsseed -nolisten?

@gavinandresen

This comment has been minimized.

Show comment Hide comment
@gavinandresen

gavinandresen Jan 31, 2012

Contributor

Testing on my Mac: I get a core dump on exit, in:

CDB::Write<std::string, CAddrMan> (this=0xb0594f20, key=@0xb0594ed0, value=@0x372560, fOverwrite=true) at db.h:106
106 int ret = pdb->put(GetTxn(), &datKey, &datValue, (fOverwrite ? 0 : DB_NOOVERWRITE));

Looks like a use-after-free problem:

(gdb) p _this
$1 = {
pdb = 0x5005da0,
strFile = {
_M_dataplus = {
std::allocator = {
<__gnu_cxx::new_allocator> = {}, },
members of std::basic_string<char,std::char_traits,std::allocator >::_Alloc_hider:
_M_p = 0x500168c "addr.dat"
}
},
vTxn = {
<std::Vector_base<DbTxn,std::allocator<DbTxn*> >> = {
_M_impl = {
std::allocator<DbTxn*> = {
<__gnu_cxx::new_allocator<DbTxn*>> = {}, },
members of std::_Vector_base<DbTxn*,std::allocator<DbTxn*> >::_Vector_impl:
_M_start = 0x0,
_M_finish = 0x0,
_M_end_of_storage = 0x0
}
}, },
fReadOnly = false
}

Contributor

gavinandresen commented Jan 31, 2012

Testing on my Mac: I get a core dump on exit, in:

CDB::Write<std::string, CAddrMan> (this=0xb0594f20, key=@0xb0594ed0, value=@0x372560, fOverwrite=true) at db.h:106
106 int ret = pdb->put(GetTxn(), &datKey, &datValue, (fOverwrite ? 0 : DB_NOOVERWRITE));

Looks like a use-after-free problem:

(gdb) p _this
$1 = {
pdb = 0x5005da0,
strFile = {
_M_dataplus = {
std::allocator = {
<__gnu_cxx::new_allocator> = {}, },
members of std::basic_string<char,std::char_traits,std::allocator >::_Alloc_hider:
_M_p = 0x500168c "addr.dat"
}
},
vTxn = {
<std::Vector_base<DbTxn,std::allocator<DbTxn*> >> = {
_M_impl = {
std::allocator<DbTxn*> = {
<__gnu_cxx::new_allocator<DbTxn*>> = {}, },
members of std::_Vector_base<DbTxn*,std::allocator<DbTxn*> >::_Vector_impl:
_M_start = 0x0,
_M_finish = 0x0,
_M_end_of_storage = 0x0
}
}, },
fReadOnly = false
}

@gavinandresen

This comment has been minimized.

Show comment Hide comment
@gavinandresen

gavinandresen Jan 31, 2012

Contributor

Also: I tested running with your patch and then running an old bitcoind; works nicely (old bitcoind thinks the addr.dat is empty, which is OK; run the new bitcoind and it thinks it needs to rewrite addr.dat again, which is also OK).

Contributor

gavinandresen commented Jan 31, 2012

Also: I tested running with your patch and then running an old bitcoind; works nicely (old bitcoind thinks the addr.dat is empty, which is OK; run the new bitcoind and it thinks it needs to rewrite addr.dat again, which is also OK).

@sipa

This comment has been minimized.

Show comment Hide comment
@sipa

sipa Feb 24, 2012

Owner

@luke-jr recently had a crash that seemed related to addrman; I am unable to reproduce the problem though. Stack traces or valgrind information from anyone who can reproduce it, is very welcome.

Owner

sipa commented Feb 24, 2012

@luke-jr recently had a crash that seemed related to addrman; I am unable to reproduce the problem though. Stack traces or valgrind information from anyone who can reproduce it, is very welcome.

CAddrMan: stochastic address manager
Design goals:
 * Only keep a limited number of addresses around, so that addr.dat does not grow without bound.
 * Keep the address tables in-memory, and occasionally write the table to addr.dat.
 * Make sure no (localized) attacker can fill the entire table with his nodes/addresses.

See comments in addrman.h for more detailed information.

@gavinandresen gavinandresen merged commit 5fee401 into bitcoin:master Mar 22, 2012

destenson pushed a commit to destenson/bitcoin--bitcoin that referenced this pull request Jun 26, 2016

+ if (nLastTry && nLastTry >= nNow-60) // never remove things tried the last minute
+ return false;
+
+ if (nTime > nNow + 10*60) // came in a flying DeLorean

This comment has been minimized.

Show comment Hide comment
@rebroad

rebroad Oct 29, 2016

Contributor

always makes me laugh this line does :) thanks :)

@rebroad

rebroad Oct 29, 2016

Contributor

always makes me laugh this line does :) thanks :)

+#define ADDRMAN_MIN_FAIL_DAYS 7
+
+// the maximum percentage of nodes to return in a getaddr call
+#define ADDRMAN_GETADDR_MAX_PCT 23

This comment has been minimized.

Show comment Hide comment
@rebroad

rebroad Oct 29, 2016

Contributor

Why 23%?

@rebroad

rebroad Oct 29, 2016

Contributor

Why 23%?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment