💬 pablomartin4btc commented on pull request "Mask values on Transactions View":
(https://github.com/bitcoin-core/gui/pull/708#discussion_r1132681562)
thanks

💬 pablomartin4btc commented on pull request "Mask values on Transactions View":
(https://github.com/bitcoin-core/gui/pull/708#discussion_r1132681928)
fixed it

👍 furszy approved a pull request: "Mask values on Transactions View"
(https://github.com/bitcoin-core/gui/pull/708)
ACK 4492de1

💬 mzumsande commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1132740863)
This looks like a nice speedup in the case where we only have a few addresses - however, due to the constant overhead of building the shuffled list of buckets in the beginning I'd expect performance to go down a bit for the case where we have many addresses to choose from. Do you see this in your benchmark?
Why does this need `ALREADY_VISITED_AND_BORING`? If we do a for-loop through a pre-shuffled list of buckets instead of a while loop, doesn't that already guarantee that we visit each bucket
...

💬 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_r1132759246)
yep :) done as in https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1131465027

💬 achow101 commented on pull request "guix: use osslsigncode 2.5":
(https://github.com/bitcoin/bitcoin/pull/27179#issuecomment-1464257858)
It appears that osslsigncode has been updated to do more verification of the signature after applying it. It now requires having a CA bundle which is not currently present in our environment. The package `nss-certs` provides these, and the option `-CAfile` needs to be given in order for osslsigncode to find the certs. The following diff resolves these issues.

```diff
diff --git a/contrib/guix/libexec/codesign.sh b/contrib/guix/libexec/codesign.sh
index f6322d761c..6ffa0f07b2 100755
--- a/c
...

💬 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#issuecomment-1464267272)
Updated with code suggestions by @MarcoFalke, dropped the unit test for now, and updated the `-debug` and `-debugexclude` config options to use the same code for consistency in behavior between them, and for consistency with the logging RPC that provides the same behavior for both `-include` and `-exclude`.

> If it's been broken forever (or at least is in all currently maintained releases), and no ones even noticed, we could take advantage of that, to remove some of the complexity here; do we
...

💬 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#issuecomment-1464341502)
Moving this back to draft to re-think the 0/none semantics.

📝 jonatack converted_to_draft a pull request: "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs"
(https://github.com/bitcoin/bitcoin/pull/27231)
The logging RPC doesn't behave as described in its help when `0` or `none` are passed.

```
$ ./src/bitcoin-cli help logging

In addition, the following are available as category names with special meanings:
- "all", "1" : represent all logging categories.
- "none", "0" : even if other logging categories are specified, ignore all of them.
```

The behavior was added in https://github.com/bitcoin/bitcoin/pull/11191, but it isn't functional in Bitcoin Core versions 22/23/24, the las
...

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1132779833)
In commit "wallet: Replace use of purpose strings with an enum" (44a2ae100b600bc9c9f4c956223fab3b1948ea8d)

Let's get rid of this unknown enum value. It looks like it was never written to disk and was purely an in-memory value. There are only two `WalletBatch::WritePurpose` calls in the codebase and if you check their callers you can see they are always passing hardcoded "send" and "receive" or "" values. This goes back to #2539 when purpose rows were first added to the database.

Having thi
...

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1132788102)
In commit "wallet: Replace use of purpose strings with an enum" (44a2ae100b600bc9c9f4c956223fab3b1948ea8d)

In #2539 there was also a ["refund"](https://github.com/bitcoin/bitcoin/blob/a41d5fe01947f2f878c055670986a165af800f9a/src/qt/paymentserver.cpp#L536) purpose, so it might make sense to add a REFUND enum value to allow those wallets to be loaded without triggering an `assert(false)` error in PurposeFromString.

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1132795600)
In commit "wallet: Replace use of purpose strings with an enum" (44a2ae100b600bc9c9f4c956223fab3b1948ea8d)

Code style comment: It is a little inconsistent to call .value() in one case but not the other. Personally, I think it would be clearest to write this as `if (purpose == SEND) ... else if (purpose == RECEIVE) else ... ` like the original code, instead of being overly verbose and paranoid about null values, which are handled sanely by [`operator==`](https://en.cppreference.com/w/cpp/utili
...

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1132814936)
In commit "wallet: Replace use of purpose strings with an enum" (44a2ae100b600bc9c9f4c956223fab3b1948ea8d)

It seems like it would be better to do something like log a warning or return an error message if an unrecognized purpose is set here, rather than crashing with an assert.

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1132816332)
In commit "wallet: Replace use of purpose strings with an enum" (44a2ae100b600bc9c9f4c956223fab3b1948ea8d)

Should this older purpose field be dropped?

💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1132822542)
In commit "wallet: Replace use of purpose strings with an enum" (44a2ae100b600bc9c9f4c956223fab3b1948ea8d)

It seems a little misleading to set default address book entry purpose to UNKNOWN, because the purpose of addresses with default entries are known: they are just change addresses used to hold avoid_reuse bookkeeping info.

So I think it would probably be better to change type of `m_purpose` field to `optional<AddressPurpose>` and leave it unset, when `m_change` is true, rather making t
...

💬 TheCharlatan commented on pull request "util: fix argsman dupe key error":
(https://github.com/bitcoin/bitcoin/pull/27236#issuecomment-1464351935)
Code review ACK 8fcbdadfad8a9b3143527141ff37e5fe1e87f3b3

💬 fanquake commented on pull request "refactor: Replace GetTimeMicros by SystemClock":
(https://github.com/bitcoin/bitcoin/pull/27233#issuecomment-1464352514)
Concept ACK

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1132837417)
good point about checking bounds. one way to do that is with simple checks like `assert(bucket <= ADDRMAN_TRIED_BUCKET_COUNT)` etc.

> Maybe change the arrays to std::array and use .at() here.

but I'd like to understand this option better. right now `vvTried` and `vvNew` are declared as C-style arrays. is your recommendation to change those `AddrManImpl` declarations so that we can utilize the bounds checking of the `.at()` function?

💬 adamjonas commented on issue "Consensus failure while upgrading bitcoind 0.8.3 > 0.22.0":
(https://github.com/bitcoin/bitcoin/issues/23913#issuecomment-1464375907)
There hasn't been forward progress on this for over a year. Appears to be a won't fix.

✅ adamjonas closed an issue: "Consensus failure while upgrading bitcoind 0.8.3 > 0.22.0"
(https://github.com/bitcoin/bitcoin/issues/23913)

💬 adamjonas commented on issue "Add CBOR RPC interface":
(https://github.com/bitcoin/bitcoin/issues/22866#issuecomment-1464379624)
There doesn't seem to be any forward progress on this feature request. Closing based on staleness.