💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1664074217)
> Why make mappings for all local IPv6 addresses?
It's the sensible choice IMO. Some routers will assign a whole bunch of routable IPv6 addresses but only allow pinholing for some. It's doesn't hurt to try.
> Also, it just occurred to me, GetLocalAddresses() operates regardless of -bind= option
Yes. But mind that `-natpmp` and `-pnp` are set to 0 if `-bind` is specified. In that case we assume the user has a more complex configuration and will just do their own mapping.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1664074217)
> Why make mappings for all local IPv6 addresses?
It's the sensible choice IMO. Some routers will assign a whole bunch of routable IPv6 addresses but only allow pinholing for some. It's doesn't hurt to try.
> Also, it just occurred to me, GetLocalAddresses() operates regardless of -bind= option
Yes. But mind that `-natpmp` and `-pnp` are set to 0 if `-bind` is specified. In that case we assume the user has a more complex configuration and will just do their own mapping.
✅ maflcko closed an issue: "Memory leak loading legacy wallet (BDB 4.8.30)"
(https://github.com/bitcoin/bitcoin/issues/27283)
(https://github.com/bitcoin/bitcoin/issues/27283)
💬 maflcko commented on issue "Memory leak loading legacy wallet (BDB 4.8.30)":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-2205913297)
Many workarounds have been mentioned in the discussion above. For example:
* disable privdb temporarily
* upgrade bdb
* suppress the warning
* remove bdb
Closing for now. I am happy to answer questions how to implement the workarounds, but I don't think there is anything that can be done in this repo that hasn't already an open pull request. If a pull request is missing, please create one, or explain exactly what should be implemented.
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-2205913297)
Many workarounds have been mentioned in the discussion above. For example:
* disable privdb temporarily
* upgrade bdb
* suppress the warning
* remove bdb
Closing for now. I am happy to answer questions how to implement the workarounds, but I don't think there is anything that can be done in this repo that hasn't already an open pull request. If a pull request is missing, please create one, or explain exactly what should be implemented.
✅ maflcko closed an issue: "Small memory leak when BerkeleyEnvironment::Open fails"
(https://github.com/bitcoin/bitcoin/issues/19034)
(https://github.com/bitcoin/bitcoin/issues/19034)
💬 maflcko commented on issue "Small memory leak when BerkeleyEnvironment::Open fails":
(https://github.com/bitcoin/bitcoin/issues/19034#issuecomment-2205913896)
Closing per https://github.com/bitcoin/bitcoin/pull/20974#issuecomment-1732362629
(https://github.com/bitcoin/bitcoin/issues/19034#issuecomment-2205913896)
Closing per https://github.com/bitcoin/bitcoin/pull/20974#issuecomment-1732362629
💬 TheCharlatan commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2205918461)
Re https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197683031
> Have a look at https://github.com/ajtowns/bitcoin/pull/9 and see what you think;
This looks good to me, thank you for implementing. Regarding disabling the buffering, I'm not sure if defaulting to off in our code is a good idea. How about this as an alternative: https://github.com/TheCharlatan/bitcoin/commit/86fed95f15fa6cee2e28679fe1ac8c537c4555a0 ? The library would always be configured to not buffer. The user co
...
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2205918461)
Re https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197683031
> Have a look at https://github.com/ajtowns/bitcoin/pull/9 and see what you think;
This looks good to me, thank you for implementing. Regarding disabling the buffering, I'm not sure if defaulting to off in our code is a good idea. How about this as an alternative: https://github.com/TheCharlatan/bitcoin/commit/86fed95f15fa6cee2e28679fe1ac8c537c4555a0 ? The library would always be configured to not buffer. The user co
...
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1664088207)
Yes, i initially had the wrong idea of what it did, should update the comment.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1664088207)
Yes, i initially had the wrong idea of what it did, should update the comment.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1664095090)
NATPMP is IPv4 only. In the case of IPv4 we don't need to explicitly bind. We don't do this for PCP either (hence passing the BIND_ANY address).
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1664095090)
NATPMP is IPv4 only. In the case of IPv4 we don't need to explicitly bind. We don't do this for PCP either (hence passing the BIND_ANY address).
💬 brunoerg commented on pull request "test: fix debug log assertion in p2p_i2p_ports test":
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2205961761)
> Ok, in the case with python3 -m http.server 60000 the reply is <!DOCTYPE HTML>. In the CI the reply is HELLO VERSION MIN=3.1 MAX=3.1, same as the request.
Yes, just did that to exemplify.
> I wonder what is a sure way to find an address and port that will fail the connection quickly? Maybe pick one of the "reserved" ports from https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers, e.g. 127.0.0.1:250? Or use the P2P port of node0 (p2p_port(0)), bitcoind is listening on it, but a
...
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2205961761)
> Ok, in the case with python3 -m http.server 60000 the reply is <!DOCTYPE HTML>. In the CI the reply is HELLO VERSION MIN=3.1 MAX=3.1, same as the request.
Yes, just did that to exemplify.
> I wonder what is a sure way to find an address and port that will fail the connection quickly? Maybe pick one of the "reserved" ports from https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers, e.g. 127.0.0.1:250? Or use the P2P port of node0 (p2p_port(0)), bitcoind is listening on it, but a
...
💬 TheCharlatan commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1664112367)
I guess the script execution cache could be wrapped just like the signature cache, which has an internal mutex.
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1664112367)
I guess the script execution cache could be wrapped just like the signature cache, which has an internal mutex.
📝 ajtowns opened a pull request: "logging: Simplify edge cases in logging configuration"
(https://github.com/bitcoin/bitcoin/pull/30384)
Corrects the documentation for logging rpc and -debugexclude to match behaviour; removes the "0" and "" entries in `LOG_CATEGORIES_BY_STR`.
(https://github.com/bitcoin/bitcoin/pull/30384)
Corrects the documentation for logging rpc and -debugexclude to match behaviour; removes the "0" and "" entries in `LOG_CATEGORIES_BY_STR`.
💬 ajtowns commented on pull request "logging: Simplify edge cases in logging configuration":
(https://github.com/bitcoin/bitcoin/pull/30384#issuecomment-2206050525)
See https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-1718621969 for history of the documented-but-not-working behaviour.
(https://github.com/bitcoin/bitcoin/pull/30384#issuecomment-2206050525)
See https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-1718621969 for history of the documented-but-not-working behaviour.
💬 ryanofsky commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2206054620)
> But that approach breaks if you're initialising the logger based on a conf file that you manage with `ArgsManager` -- since `ArgsManager` needs to initialise its `cs_args`, which then needs a logger, but you're still trying to work out how to configure the logger at this point.
>
> It also breaks for the various global mutexes that are still in the kernel (just `cs_main`, `g_best_block_mutex`? perhaps `DumpMutex()::dump_mutex`).
I don't think anything is broken here, or maybe we have dif
...
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2206054620)
> But that approach breaks if you're initialising the logger based on a conf file that you manage with `ArgsManager` -- since `ArgsManager` needs to initialise its `cs_args`, which then needs a logger, but you're still trying to work out how to configure the logger at this point.
>
> It also breaks for the various global mutexes that are still in the kernel (just `cs_main`, `g_best_block_mutex`? perhaps `DumpMutex()::dump_mutex`).
I don't think anything is broken here, or maybe we have dif
...
💬 ryanofsky commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2206072773)
I wonder if there is a DraftBot parsing bug? It seems to be parsing https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2154910604 as a concept ack instead of a plain ACK, and it requested another review from me despite the ACK.
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2206072773)
I wonder if there is a DraftBot parsing bug? It seems to be parsing https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2154910604 as a concept ack instead of a plain ACK, and it requested another review from me despite the ACK.
🤔 dergoegge reviewed a pull request: "Stratum v2 Template Provider (take 3)"
(https://github.com/bitcoin/bitcoin/pull/29432#pullrequestreview-2156406658)
Thank you for all the work you've put into this.
Approach NACK
I refuse to believe that re-implementing the entire networking stack, adding a noise protocol implementation and a new networking protocol is the best solution to having a non-RPC/non-poll based template provider.
After looking at this PR, I would propose the following interface additions to Bitcoin Core:
* We add a new zmq publisher, e.g. `-zmqpubtemplate`, which publishes block templates as soon as they become available
...
(https://github.com/bitcoin/bitcoin/pull/29432#pullrequestreview-2156406658)
Thank you for all the work you've put into this.
Approach NACK
I refuse to believe that re-implementing the entire networking stack, adding a noise protocol implementation and a new networking protocol is the best solution to having a non-RPC/non-poll based template provider.
After looking at this PR, I would propose the following interface additions to Bitcoin Core:
* We add a new zmq publisher, e.g. `-zmqpubtemplate`, which publishes block templates as soon as they become available
...
💬 dergoegge commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1664165837)
How is this not polling?
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1664165837)
How is this not polling?
💬 ryanofsky commented on pull request "logging: Simplify edge cases in logging configuration":
(https://github.com/bitcoin/bitcoin/pull/30384#issuecomment-2206082134)
Will review, but #29798 is making a similar and maybe wider simplification, so you might want to check that out.
(https://github.com/bitcoin/bitcoin/pull/30384#issuecomment-2206082134)
Will review, but #29798 is making a similar and maybe wider simplification, so you might want to check that out.
💬 vasild commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2206100324)
@Sjors, I changed the test `sv2_connman_tests/client_tests` from https://github.com/bitcoin/bitcoin/pull/30332 to use mocked sockets instead of real ones from the operating system. See the top commit from here: https://github.com/vasild/bitcoin/commits/sv2_mocksock/, that compiles, but the test fails at some point. I will try to get it to pass, just sharing this early wip with you.
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2206100324)
@Sjors, I changed the test `sv2_connman_tests/client_tests` from https://github.com/bitcoin/bitcoin/pull/30332 to use mocked sockets instead of real ones from the operating system. See the top commit from here: https://github.com/vasild/bitcoin/commits/sv2_mocksock/, that compiles, but the test fails at some point. I will try to get it to pass, just sharing this early wip with you.
💬 ajtowns commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2206100735)
> > But that approach breaks if you're initialising the logger based on a conf file that you manage with `ArgsManager` -- since `ArgsManager` needs to initialise its `cs_args`, which then needs a logger, but you're still trying to work out how to configure the logger at this point.
> > It also breaks for the various global mutexes that are still in the kernel (just `cs_main`, `g_best_block_mutex`? perhaps `DumpMutex()::dump_mutex`).
>
> I don't think anything is broken here, or maybe we have
...
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2206100735)
> > But that approach breaks if you're initialising the logger based on a conf file that you manage with `ArgsManager` -- since `ArgsManager` needs to initialise its `cs_args`, which then needs a logger, but you're still trying to work out how to configure the logger at this point.
> > It also breaks for the various global mutexes that are still in the kernel (just `cs_main`, `g_best_block_mutex`? perhaps `DumpMutex()::dump_mutex`).
>
> I don't think anything is broken here, or maybe we have
...
📝 furszy opened a pull request: "p2p: send not_found msgs for unknown, pruned or unwilling to share blocks"
(https://github.com/bitcoin/bitcoin/pull/30385)
The 'not_found' message, introduced in protocol version 70001 (Bitcoin Core 0.8.0 - Feb 2013), serves as a response to a 'getdata' message when the receiving node does not have the requested data available for relay. Thus far, this has only been applied to transactions. This PR proposes extending its use to blocks as well.
Currently, when the remote peer does not have the requested block data, it ignores the 'getdata' inventory block request. This lack of response causes the requesting node t
...
(https://github.com/bitcoin/bitcoin/pull/30385)
The 'not_found' message, introduced in protocol version 70001 (Bitcoin Core 0.8.0 - Feb 2013), serves as a response to a 'getdata' message when the receiving node does not have the requested data available for relay. Thus far, this has only been applied to transactions. This PR proposes extending its use to blocks as well.
Currently, when the remote peer does not have the requested block data, it ignores the 'getdata' inventory block request. This lack of response causes the requesting node t
...