💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1146874799)
s/v25/v26/
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1146874799)
s/v25/v26/
💬 theuni commented on pull request "fix: contrib: allow multi-sig binary verification":
(https://github.com/bitcoin/bitcoin/pull/23020#issuecomment-1481930724)
> https://github.com/achow101/bitcoin/tree/pr23020-direct-bins-gpg-parse is a branch which implements a subcommand for verifying binaries directly and to use `--status-file` for the machine parseable output.
Testing your branch now. Let me know if you'd like to take review elsewhere.
Sorry I don't speak much python. Some of these problems may already exist as opposed to being introduced here.
First, testing with python3.8:
`verify.py pub 23.0-x86_64`
```bash
File "/home/cory/dev/bi
...
(https://github.com/bitcoin/bitcoin/pull/23020#issuecomment-1481930724)
> https://github.com/achow101/bitcoin/tree/pr23020-direct-bins-gpg-parse is a branch which implements a subcommand for verifying binaries directly and to use `--status-file` for the machine parseable output.
Testing your branch now. Let me know if you'd like to take review elsewhere.
Sorry I don't speak much python. Some of these problems may already exist as opposed to being introduced here.
First, testing with python3.8:
`verify.py pub 23.0-x86_64`
```bash
File "/home/cory/dev/bi
...
💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1146876222)
Remove
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1146876222)
Remove
💬 ajtowns commented on pull request "p2p: remove adjusted time":
(https://github.com/bitcoin/bitcoin/pull/25908#issuecomment-1481946114)
> Rebased on master. @ajtowns putting aside the current comments, any thoughts on including changes like this in bitcoin-inquisition?
No objection to it, but not sure why it would be easier to do it that way than just merge into master?
(https://github.com/bitcoin/bitcoin/pull/25908#issuecomment-1481946114)
> Rebased on master. @ajtowns putting aside the current comments, any thoughts on including changes like this in bitcoin-inquisition?
No objection to it, but not sure why it would be easier to do it that way than just merge into master?
💬 achow101 commented on pull request "fix: contrib: allow multi-sig binary verification":
(https://github.com/bitcoin/bitcoin/pull/23020#issuecomment-1481979695)
> I probably won't have time in the next few days to attend to this, so feel free to open a replacement PR with @achow101's branch if I'm holding anything up.
Unfortunately I will be away for the next two weeks soon, so I won't have any time to maintain a replacement PR either.
> First, testing with python3.8: `verify.py pub 23.0-x86_64`
>
> ```shell
> File "/home/cory/dev/bitcoin2/contrib/verifybinaries/verify.py", line 192, in parse_gpg_result
> def line_begins_with(patt: str,
...
(https://github.com/bitcoin/bitcoin/pull/23020#issuecomment-1481979695)
> I probably won't have time in the next few days to attend to this, so feel free to open a replacement PR with @achow101's branch if I'm holding anything up.
Unfortunately I will be away for the next two weeks soon, so I won't have any time to maintain a replacement PR either.
> First, testing with python3.8: `verify.py pub 23.0-x86_64`
>
> ```shell
> File "/home/cory/dev/bitcoin2/contrib/verifybinaries/verify.py", line 192, in parse_gpg_result
> def line_begins_with(patt: str,
...
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1146930797)
```suggestion
// As long as target feerate is below tx6's ancestor feerate, there is no bump fee.
```
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1146930797)
```suggestion
// As long as target feerate is below tx6's ancestor feerate, there is no bump fee.
```
💬 theStack commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1146941331)
minor suggestion: in the unit test's `sanity_check` function, could also check that all (positive) bumpfee entries refer to one of the passed tx's:
```suggestion
// No negative bumpfees. All positive bumpfees must refer to one of the passed tx's outputs.
for (const auto& [outpoint, fee] : bumpfees) {
if (fee < 0) return false;
if (fee == 0) continue;
auto outpoint_ = outpoint; // structured bindings can't be captured in C++17, so we need to use a variable
...
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1146941331)
minor suggestion: in the unit test's `sanity_check` function, could also check that all (positive) bumpfee entries refer to one of the passed tx's:
```suggestion
// No negative bumpfees. All positive bumpfees must refer to one of the passed tx's outputs.
for (const auto& [outpoint, fee] : bumpfees) {
if (fee < 0) return false;
if (fee == 0) continue;
auto outpoint_ = outpoint; // structured bindings can't be captured in C++17, so we need to use a variable
...
💬 theStack commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1146927271)
nit: Seems like this pair of curly-braces and the extra-indent is a left-over from earlier code (probably involving a `LOCK`?) and can just be removed.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1146927271)
nit: Seems like this pair of curly-braces and the extra-indent is a left-over from earlier code (probably involving a `LOCK`?) and can just be removed.
💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1146954107)
> Seems kind of weird to change the CLI to use a "test-only" RPC in any case
hm, that's a good point. I originally suggested having the RPC as hidden since I imagine this feature to mostly be used by bitcoin contributors or super-users. but I'd imagine `-addrinfo` to be more accessible. so the two obvious options are to either (1) make the RPC public (2) leave `-addrinfo` as is.
I think I'd lean towards (1) because since it improves the results of `-addrinfo` , but also depends on the out
...
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1146954107)
> Seems kind of weird to change the CLI to use a "test-only" RPC in any case
hm, that's a good point. I originally suggested having the RPC as hidden since I imagine this feature to mostly be used by bitcoin contributors or super-users. but I'd imagine `-addrinfo` to be more accessible. so the two obvious options are to either (1) make the RPC public (2) leave `-addrinfo` as is.
I think I'd lean towards (1) because since it improves the results of `-addrinfo` , but also depends on the out
...
💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1146956058)
that's interesting. are you talking about the use case where clients would upgrade their bitcoin-cli but not their bitcoind?
I'm curious to learn more about this, do you have additional context you can share? eg. is this a frequent use case? are there other circumstances / PRs where something similar was handled? are cli updates then versioned, or the expectation is to maintain backward compatibility from the point in time that the new interface is exposed?
I'm not very familiar with the
...
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1146956058)
that's interesting. are you talking about the use case where clients would upgrade their bitcoin-cli but not their bitcoind?
I'm curious to learn more about this, do you have additional context you can share? eg. is this a frequent use case? are there other circumstances / PRs where something similar was handled? are cli updates then versioned, or the expectation is to maintain backward compatibility from the point in time that the new interface is exposed?
I'm not very familiar with the
...
💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1146964856)
Yes, for -netinfo we've needed to pay attention to this after feedback from affected users, including colleagues here, who were running nodes on previous, supported versions of Bitcoin Core, either as the nodes were long-running or for benchmarking or other reasons and requested we avoid breaking user space.
One solution could be to do a server version check (there is one for -netinfo) to decide which RPC to call. Another would be to leave -addrinfo as-is and (maybe, just an idea) add the ne
...
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1146964856)
Yes, for -netinfo we've needed to pay attention to this after feedback from affected users, including colleagues here, who were running nodes on previous, supported versions of Bitcoin Core, either as the nodes were long-running or for benchmarking or other reasons and requested we avoid breaking user space.
One solution could be to do a server version check (there is one for -netinfo) to decide which RPC to call. Another would be to leave -addrinfo as-is and (maybe, just an idea) add the ne
...
💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1146968261)
(For -netinfo, as it continued to call the same endpoints, maintaining compatibility meant adding `IsNull()` checks on some getpeerinfo fields that were newer or that changed from always returned to optional.)
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1146968261)
(For -netinfo, as it continued to call the same endpoints, maintaining compatibility meant adding `IsNull()` checks on some getpeerinfo fields that were newer or that changed from always returned to optional.)
💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1146970653)
> since it improves the results of `-addrinfo`
> how important are IsTerrible addresses, and would it be interesting to have the breakdown with/without that filtering.
It might be interesting to have both without and without IsTerrible() filtering to be able to compare over time, either by returning a breakdown in the new RPC or by leaving -addrinfo unchanged to compare the two.
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1146970653)
> since it improves the results of `-addrinfo`
> how important are IsTerrible addresses, and would it be interesting to have the breakdown with/without that filtering.
It might be interesting to have both without and without IsTerrible() filtering to be able to compare over time, either by returning a breakdown in the new RPC or by leaving -addrinfo unchanged to compare the two.
💬 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_r1146970997)
Should say "UNSUPPORTED", not "DEPRECATED" ?
Weak NACK on removing this option -- in [inq#23](https://github.com/bitcoin-inquisition/bitcoin/pull/23#discussion_r1139697940) we've got a use case for accepting txs into the mempool that we don't intend to mine (until they are CPFP'ed).
(https://github.com/bitcoin/bitcoin/pull/27018#discussion_r1146970997)
Should say "UNSUPPORTED", not "DEPRECATED" ?
Weak NACK on removing this option -- in [inq#23](https://github.com/bitcoin-inquisition/bitcoin/pull/23#discussion_r1139697940) we've got a use case for accepting txs into the mempool that we don't intend to mine (until they are CPFP'ed).
💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1146972607)
For example, I've been tweeting a bit about the recent increase in Tor and I2P peers known to my node based on -addrinfo output and mentioning that these were also recently active peer counts (active in the last month).
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1146972607)
For example, I've been tweeting a bit about the recent increase in Tor and I2P peers known to my node based on -addrinfo output and mentioning that these were also recently active peer counts (active in the last month).
💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1146945075)
`all_entries` is not used anywhere.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1146945075)
`all_entries` is not used anywhere.
💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1146944495)
This is just a nit but I wouldn't call this `tx5_feerate`, it's the `tx5_tx6_package_feerate`.
At least in my head, when we talk about certain tx feerate, we talk abou the relation between the tx ancestors fee and their size (including the tx itself). Which does not include the descendants (in this case, tx6)
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1146944495)
This is just a nit but I wouldn't call this `tx5_feerate`, it's the `tx5_tx6_package_feerate`.
At least in my head, when we talk about certain tx feerate, we talk abou the relation between the tx ancestors fee and their size (including the tx itself). Which does not include the descendants (in this case, tx6)
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1482054469)
Updated d00762c75a1b9ad16e0dadf25886a7baefa84a12 -> 37ffeca9e0363781f8168114faad004d5543260f ([`pr/ignoredconf.7`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.7) -> [`pr/ignoredconf.8`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ignoredconf.7..pr/ignoredconf.8)) disabling one of the new tests on win64 since it continues to fail https://cirrus-ci.com/task/5859482064912384
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1482054469)
Updated d00762c75a1b9ad16e0dadf25886a7baefa84a12 -> 37ffeca9e0363781f8168114faad004d5543260f ([`pr/ignoredconf.7`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.7) -> [`pr/ignoredconf.8`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ignoredconf.7..pr/ignoredconf.8)) disabling one of the new tests on win64 since it continues to fail https://cirrus-ci.com/task/5859482064912384
💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1482065109)
added a commit that increases fuzz coverage for select by network
@brunoerg
> Also, should we have (at least) one anchor per network instead of only two "generic" ones?
yeah, def think it's worth considering in relation to increasing block-relay-peers. @mzumsande & I have started brainstorming & plan to open an issue once we formulate some initial ideas. in the meanwhile, feel free to reach out directly if you're interested in discussing further :)
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1482065109)
added a commit that increases fuzz coverage for select by network
@brunoerg
> Also, should we have (at least) one anchor per network instead of only two "generic" ones?
yeah, def think it's worth considering in relation to increasing block-relay-peers. @mzumsande & I have started brainstorming & plan to open an issue once we formulate some initial ideas. in the meanwhile, feel free to reach out directly if you're interested in discussing further :)
💬 theStack commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1147032833)
This fee delta seems excessive (leading to a fee-rate of >1 million sats/vbyte for tx6), I guess just using one of `{high,normal,low}_fee` should also be fine? Then the testing code-path of getting a bumpfee for tx5/tx6 would be hit (see also comment below).
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1147032833)
This fee delta seems excessive (leading to a fee-rate of >1 million sats/vbyte for tx6), I guess just using one of `{high,normal,low}_fee` should also be fine? Then the testing code-path of getting a bumpfee for tx5/tx6 would be hit (see also comment below).