💬 achow101 commented on pull request "rpc: getdescriptorinfo also returns normalized descriptor":
(https://github.com/bitcoin/bitcoin/pull/29396#issuecomment-1935265831)
> I'm getting an error in one the functional tests cases in `test/functional/rpc_getdescriptorinfo.py`.
>
> A fix would be changing the derivation path from `1/2h` (hardened) to `1/2` (non-hardened):
>
> ```
> - self.test_desc("pkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1h/2)", isrange=False, issolvable=True, hasprivatekeys=False)
> + self.test_desc("pkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Po
...

💬 epiccurious commented on pull request "fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse`":
(https://github.com/bitcoin/bitcoin/pull/29413#issuecomment-1935284702)
If a length of 32 is insufficient, why did you choose 1000 vs 100 or 10,000?

💬 epiccurious commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#issuecomment-1935287441)
How do you define a mutated block? What are the known forms of mutated blocks?

💬 daniel-btc-nym commented on issue "getJsonToken assumes underlying string is null-terminated but requires end pointer":
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1935289038)
Yeah, I ran some valgrind tests before and after my changes and it clearly showed the "uninitialized" accesses when we copy the data first into a tight vector as you suggested.

I added more test coverage and think this should be enough for this issue.

What do you think about [the changes](https://github.com/bitcoin/bitcoin/compare/master...daniel-btc-nym:bitcoin:master)

If they look OK, I can merge them into a single commit -- (I suppose by forking again?) -- and then making a pull
...

💬 epiccurious commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1935289598)
Concept ACK f4db3546c003822e485f1597701f278eef197619.

💬 epiccurious commented on pull request "build: Add missed definition for `AM_OBJCXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1935291945)
utACK 2fb758da581b5cefcc0232e1b40f13143ad2bdaf

💬 araujo88 commented on pull request "rpc: getdescriptorinfo also returns normalized descriptor":
(https://github.com/bitcoin/bitcoin/pull/29396#issuecomment-1935314945)
> > I'm getting an error in one the functional tests cases in `test/functional/rpc_getdescriptorinfo.py`.
> > A fix would be changing the derivation path from `1/2h` (hardened) to `1/2` (non-hardened):
> > ```
> > - self.test_desc("pkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1h/2)", isrange=False, issolvable=True, hasprivatekeys=False)
> > + self.test_desc("pkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3
...

📝 hernanmarino opened a pull request: "gui: Correct tooltip wording for watch-only wallets"
(https://github.com/bitcoin-core/gui/pull/792)
This is a continuation of https://github.com/bitcoin-core/gui/pull/37
This is a simple change, but imho necessary because it leads to confusion.
The objective of this change is to provide a better description for the tooltip for the "Available Balance" balance because the current version on master mentions the word "You current spendable balance" which is a bit misleading, because that balance is not always spendable , or not always yours on watch only wallets.

💬 hernanmarino commented on pull request "gui: Correct tooltip wording for watch-only wallets":
(https://github.com/bitcoin-core/gui/pull/792#issuecomment-1935323075)
As a first commit, I only took @ saibato's latest commit and corrected a couple of bugs (mentioned by @pablomartin4btc in the original PR.)
Marked as Draft, in the following days I'll try to address all comments and sugestions in that PR, and mark it ready for review.

💬 hernanmarino commented on pull request "indicate explicit to the user that the wallet balances shown is watch only.":
(https://github.com/bitcoin-core/gui/pull/37#issuecomment-1935326761)
Hi, if no one disagrees, I am taking over the development of this functionality. Discussion can continue here: https://github.com/bitcoin-core/gui/pull/792 Thanks @Saibato for the work

🤔 stratospher reviewed a pull request: "rpc: parse legacy pubkeys consistently with specific error messages"
(https://github.com/bitcoin/bitcoin/pull/28336#pullrequestreview-1871661382)
Concept ACK! liked how the error handling is done in a consitent manner now.

💬 stratospher commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1483864342)
b8ee41b: is it impossible to have valid bitcoin addresses which are just hex characters?

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1483975288)
I see thanks for the explanation. It seems to me like most of these cases should be caught by the optimization to cut instead of shift when we exceed the weight:

> opt: Cut if last addition was minimal weight
>
> In situations where we have UTXO groups of various weight, we can CUT
> rather than SHIFT when we exceeded the max_weight or the best
> selection’s weight while the last step was equal to the minimum weight
> in the lookahead.

It would take more time to determine whether th
...

💬 maflcko commented on pull request "fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse`":
(https://github.com/bitcoin/bitcoin/pull/29413#issuecomment-1935504931)
lgtm ACK 864e2e9097de8f1fda63137f803687dd5cc96c03

💬 maflcko commented on pull request "doc: Update translation process guide":
(https://github.com/bitcoin/bitcoin/pull/29414#discussion_r1483999946)
Instead of copy-pasting third-party docs, it may be better to just update the link to the third-party docs and remove the outdated parts, to reduce the maintenance overhead here.

💬 maflcko commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1484005817)
If node 0 can load the snapshot, that sounds like a bug, because node 0 created the snapshot, no?

💬 TheCharlatan commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1935522411)
Made this into a scripted-diff here: https://github.com/TheCharlatan/bitcoin/commit/e4186f771294a8b112bac759f92ea5e4bef8f168

There are no differences with the patch in this PR, besides a single whitespace issue. Feel free to pick the commit and make this into a scripted-diff as well, though I'm not sure if it is worth it, since the script is a bit dense.

💬 maflcko commented on pull request "Nuke adjusted time from validation (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1935525053)
> My belief we should conserve a a warning system based on other peers nversion announcements.

Yes, this is exactly what this pull request did. See also the description, which said: "while the warning to users if their clock is out of sync with the rest of the network remains."

See also: https://bitcoinops.org/en/newsletters/2024/02/07/#bitcoin-core-28956

💬 maflcko commented on issue "getJsonToken assumes underlying string is null-terminated but requires end pointer":
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1935535162)
I like bugfixes the most, when they are touching only a single line of code. But if more changes are required, then that is fine, too.

I checked that this is only a theoretical issue in Bitcoin Core, and can not be hit in production code, but it may still be good to fix it.

It may be good to update the fuzz target to catch the issue here: `src/test/fuzz/parse_univalue.cpp`

💬 0xB10C commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#issuecomment-1935600643)
Thanks for the review @sr-gi! You might want to take a look at #29007 first - this allows us to also test `failed-adding-to-tried`. Once this is merged, I'll continue to work on this PR.