💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1163319528)
I left them without quotes as none of the other category values in the -debug and -debugexclude helps are in quotes. But you may be right that it's nevertheless clearer to add them here. Done.
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1163319528)
I left them without quotes as none of the other category values in the -debug and -debugexclude helps are in quotes. But you may be right that it's nevertheless clearer to add them here. Done.
💬 mzumsande commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1504081199)
> I am not sure about CJDNS. Normally I would say treat it like Tor and I2P (but I even had the heretic idea to do that with IPv4 and IPv6 too, so maybe I lean a bit to the paranoid side, don't listen to me too much). However there are very few CJDNS addresses right now on mainnet and if we only advertise our CJDNS address to CJDNS peers that could impede its adoption.
Makes sense to me - if bitcoin over CJDNS gets more popular we could revisit this later.
> Ultimately, the perfect soluti
...
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1504081199)
> I am not sure about CJDNS. Normally I would say treat it like Tor and I2P (but I even had the heretic idea to do that with IPv4 and IPv6 too, so maybe I lean a bit to the paranoid side, don't listen to me too much). However there are very few CJDNS addresses right now on mainnet and if we only advertise our CJDNS address to CJDNS peers that could impede its adoption.
Makes sense to me - if bitcoin over CJDNS gets more popular we could revisit this later.
> Ultimately, the perfect soluti
...
📝 theStack opened a pull request: "doc: update OpenBSD build docs for 7.3 (external signer support available)"
(https://github.com/bitcoin/bitcoin/pull/27449)
With OpenBSD 7.3, the waitid(2) system call is implemented (see https://github.com/openbsd/src/commit/8112871f19bbd25e86c93d0f901071ca2335a352, first mentioned kernel improvement at https://www.openbsd.org/73.html).
This means Boost.Process finally doesn't fail to compile anymore and we can remove the build hint about missing external signer support. Tested on my amd64 machine by reconfiguring / rebuilding master branch and successfully running the functional test wallet_signer.py. :heavy_che
...
(https://github.com/bitcoin/bitcoin/pull/27449)
With OpenBSD 7.3, the waitid(2) system call is implemented (see https://github.com/openbsd/src/commit/8112871f19bbd25e86c93d0f901071ca2335a352, first mentioned kernel improvement at https://www.openbsd.org/73.html).
This means Boost.Process finally doesn't fail to compile anymore and we can remove the build hint about missing external signer support. Tested on my amd64 machine by reconfiguring / rebuilding master branch and successfully running the functional test wallet_signer.py. :heavy_che
...
💬 rsafier commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1504092196)
Concept ACK. Been wanting this for a while
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1504092196)
Concept ACK. Been wanting this for a while
💬 mzumsande commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163340252)
This fails the linter, could use `ParseInt64()`.
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163340252)
This fails the linter, could use `ParseInt64()`.
💬 mzumsande commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163341355)
Maybe require that `signetblocktime > 0` ? Probably bad things would happen otherwise.
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163341355)
Maybe require that `signetblocktime > 0` ? Probably bad things would happen otherwise.
💬 benthecarman commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163356429)
Did `signetblocktime >= 0` just to be sure
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163356429)
Did `signetblocktime >= 0` just to be sure
💬 ajtowns commented on pull request "mempool / miner: regularly flush <=0-fee entries and mine everything in the mempool":
(https://github.com/bitcoin/bitcoin/pull/27018#discussion_r1163371721)
From the ephemeral anchors review club:
> `<glozow> ok hm. i kinda need to get rid of blockmintxfee for pakcage relay tho`
Can you elaborate on that? You can just drop the blockminfee commit from this PR, and it seems fine, no?
The only result of setting `-blockminfee` is that if its value is higher than `-minrelaytxfee` you'll potentially never empty your own memory even if you're the only miner and no new transactions are being created/relayed. That's undesirable as default behaviour,
...
(https://github.com/bitcoin/bitcoin/pull/27018#discussion_r1163371721)
From the ephemeral anchors review club:
> `<glozow> ok hm. i kinda need to get rid of blockmintxfee for pakcage relay tho`
Can you elaborate on that? You can just drop the blockminfee commit from this PR, and it seems fine, no?
The only result of setting `-blockminfee` is that if its value is higher than `-minrelaytxfee` you'll potentially never empty your own memory even if you're the only miner and no new transactions are being created/relayed. That's undesirable as default behaviour,
...
💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163430354)
Yes, I've changed `GetUpdatedStateFn` to `TryUpdatingStateFn`, and `get_updated_state` to `try_updating_state`.
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163430354)
Yes, I've changed `GetUpdatedStateFn` to `TryUpdatingStateFn`, and `get_updated_state` to `try_updating_state`.
💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163430929)
I've implemented this, it also fixes the CI error.
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163430929)
I've implemented this, it also fixes the CI error.
💬 ajtowns commented on pull request "Deprecate and remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#discussion_r1163580752)
"Deprecate" means "we still support this, but we recommend you don't use it". cf https://en.wikipedia.org/wiki/Deprecation
(https://github.com/bitcoin/bitcoin/pull/27426#discussion_r1163580752)
"Deprecate" means "we still support this, but we recommend you don't use it". cf https://en.wikipedia.org/wiki/Deprecation
💬 ajtowns commented on pull request "Deprecate and remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1504598010)
> However it is no longer used (?), is bad for privacy, and we can simplify our P2P code by removing it.
Not knowing whether it's used seems a bad starting point? And it's only an extra boolean and 20 something lines of code, which doesn't seem like a big deal? I would have expected it to be kept around until BIP 37 (bloom filtering) support was dropped entirely, but don't really have a justification for that.
> let's say we came up with a way [...] to try to sync the top of node mempools
...
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1504598010)
> However it is no longer used (?), is bad for privacy, and we can simplify our P2P code by removing it.
Not knowing whether it's used seems a bad starting point? And it's only an extra boolean and 20 something lines of code, which doesn't seem like a big deal? I would have expected it to be kept around until BIP 37 (bloom filtering) support was dropped entirely, but don't really have a justification for that.
> let's say we came up with a way [...] to try to sync the top of node mempools
...
💬 MarcoFalke commented on pull request "Deprecate and remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1504787771)
> Syncing core nodes by sending INV messages and doing regular tx relay effectively rebroadcasts the transactions, potentially resetting their expiry time. Having either a p2p command or an rpc command that lets you just sync mempools without necessarily also rebroadcasting the different transactions would be nice.
Maybe I am missing something, but I fail to see how the expiry time can be synced over p2p. My understanding is that the receiving node sets the expiry for newly accepted transacti
...
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1504787771)
> Syncing core nodes by sending INV messages and doing regular tx relay effectively rebroadcasts the transactions, potentially resetting their expiry time. Having either a p2p command or an rpc command that lets you just sync mempools without necessarily also rebroadcasting the different transactions would be nice.
Maybe I am missing something, but I fail to see how the expiry time can be synced over p2p. My understanding is that the receiving node sets the expiry for newly accepted transacti
...
💬 MarcoFalke commented on pull request "RPC: Add universal options argument to listtransactions":
(https://github.com/bitcoin/bitcoin/pull/22807#issuecomment-1504792765)
Needs rebase
(https://github.com/bitcoin/bitcoin/pull/22807#issuecomment-1504792765)
Needs rebase
💬 MarcoFalke commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163735086)
> This fails the linter, could use ParseInt64().
Is there a reason to not just use `GetIntArg` directly? If not, for new code, `ToIntegral` should be used, unless the legacy `ParseInt` behavior of accepting a leading `+` is needed?
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163735086)
> This fails the linter, could use ParseInt64().
Is there a reason to not just use `GetIntArg` directly? If not, for new code, `ToIntegral` should be used, unless the legacy `ParseInt` behavior of accepting a leading `+` is needed?
💬 MarcoFalke commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163736613)
Any reason to leak implementation details (`__func__`) to the user when it doesn't add any value? `-signetblocktime must be greater than 0` should be self-explanatory?
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163736613)
Any reason to leak implementation details (`__func__`) to the user when it doesn't add any value? `-signetblocktime must be greater than 0` should be self-explanatory?
💬 sdaftuar commented on pull request "Deprecate and remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1504865608)
> If you control both nodes, wouldn't it be easier to run the RPC on the sending side directly to fan out all transactions instead of going an extra hop over P2P?
@MarcoFalke Yeah I think this is a fair point... And I guess really if there were a use case, it would be for users of other software interacting with Bitcoin Core and wanting some functionality like this without having to recompile their Bitcoin Core node to run a custom patch (a situation that does not apply to me!).
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1504865608)
> If you control both nodes, wouldn't it be easier to run the RPC on the sending side directly to fan out all transactions instead of going an extra hop over P2P?
@MarcoFalke Yeah I think this is a fair point... And I guess really if there were a use case, it would be for users of other software interacting with Bitcoin Core and wanting some functionality like this without having to recompile their Bitcoin Core node to run a custom patch (a situation that does not apply to me!).
👍 fanquake approved a pull request: "doc: update OpenBSD build docs for 7.3 (external signer support available)"
(https://github.com/bitcoin/bitcoin/pull/27449#pullrequestreview-1380822649)
ACK 6b17994ede6fe1961667d2e96127291b2a8b4f9d - haven't tested, but looks good to me.
(https://github.com/bitcoin/bitcoin/pull/27449#pullrequestreview-1380822649)
ACK 6b17994ede6fe1961667d2e96127291b2a8b4f9d - haven't tested, but looks good to me.
💬 josibake commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#issuecomment-1504894418)
reACK https://github.com/bitcoin/bitcoin/commit/18fc71a3adee5de0f74ba6ff18f5ed31ba79a646
(https://github.com/bitcoin/bitcoin/pull/27217#issuecomment-1504894418)
reACK https://github.com/bitcoin/bitcoin/commit/18fc71a3adee5de0f74ba6ff18f5ed31ba79a646
🚀 fanquake merged a pull request: "wallet: Replace use of purpose strings with an enum"
(https://github.com/bitcoin/bitcoin/pull/27217)
(https://github.com/bitcoin/bitcoin/pull/27217)