✅ brunoerg closed a pull request: "wallet: add config to prioritize a solution that doesn't create change in coin selection "
(https://github.com/bitcoin/bitcoin/pull/23475)
(https://github.com/bitcoin/bitcoin/pull/23475)
💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1481881156)
Not sure why the functional tests are failing. The test failing on the Win64 job is passing on the 32-bit job and vice-versa. Both are passing locally.
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1481881156)
Not sure why the functional tests are failing. The test failing on the Win64 job is passing on the 32-bit job and vice-versa. Both are passing locally.
💬 achow101 commented on pull request "bench: update logging benchmarks":
(https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1481883224)
ACK 8c47d599b87d6b2d43e7d37ce0aaf4f541535bb9
(https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1481883224)
ACK 8c47d599b87d6b2d43e7d37ce0aaf4f541535bb9
🚀 achow101 merged a pull request: "bench: update logging benchmarks"
(https://github.com/bitcoin/bitcoin/pull/26957)
(https://github.com/bitcoin/bitcoin/pull/26957)
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1481906438)
Updated 972335762aeaab557dbb2e44b149b18005987f8b -> d00762c75a1b9ad16e0dadf25886a7baefa84a12 ([`pr/ignoredconf.6`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.6) -> [`pr/ignoredconf.7`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.7), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ignoredconf.6..pr/ignoredconf.7)) to try to fix a new win64 CI error in test_ignored_default_conf:
```
Traceback (most recent call last):
File "C:\Users\ContainerAdminis
...
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1481906438)
Updated 972335762aeaab557dbb2e44b149b18005987f8b -> d00762c75a1b9ad16e0dadf25886a7baefa84a12 ([`pr/ignoredconf.6`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.6) -> [`pr/ignoredconf.7`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.7), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ignoredconf.6..pr/ignoredconf.7)) to try to fix a new win64 CI error in test_ignored_default_conf:
```
Traceback (most recent call last):
File "C:\Users\ContainerAdminis
...
💬 ryanofsky commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1481912454)
Win64 wallet_create_tx.py failure is probably fixed by #27318
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1481912454)
Win64 wallet_create_tx.py failure is probably fixed by #27318
💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1146864188)
Thanks @vasild. It could maybe even be split further into two methods, one for the `warning` deprecations in 4 wallet RPCs, and one called by all the wallet RPCs having a `warnings` field. Will explore.
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1146864188)
Thanks @vasild. It could maybe even be split further into two methods, one for the `warning` deprecations in 4 wallet RPCs, and one called by all the wallet RPCs having a `warnings` field. Will explore.
💬 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.