💬 jonatack commented on pull request "rpc, util: deduplicate AmountFromValue() using util::Result":
(https://github.com/bitcoin/bitcoin/pull/28134#discussion_r1276671966)
That would encompass many of the methods in `rpc/util`. Where would you suggest to put them?
(https://github.com/bitcoin/bitcoin/pull/28134#discussion_r1276671966)
That would encompass many of the methods in `rpc/util`. Where would you suggest to put them?
💬 jonatack commented on pull request "rpc, util: deduplicate AmountFromValue() using util::Result":
(https://github.com/bitcoin/bitcoin/pull/28134#issuecomment-1654195359)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/28134#issuecomment-1654195359)
Rebased.
💬 luke-jr commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1276717254)
Shouldn't we still execute the notification? While it's not possible for the client to confirm it worked, it should still execute...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1276717254)
Shouldn't we still execute the notification? While it's not possible for the client to confirm it worked, it should still execute...
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1276726382)
My thinking was that since our RPC API does not have any notifications defined yet we could discard them for now. Like, our RPCs and responses are a separate category from notifications. But you have an interesting point, a user could send a notification with a method like `generatetoaddress` and just not want to wait for a reply. There would be plenty of "getter" RPCs that would make no sense in a notification however and just waste time on the server. Maybe this is best suited for a follow-up
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1276726382)
My thinking was that since our RPC API does not have any notifications defined yet we could discard them for now. Like, our RPCs and responses are a separate category from notifications. But you have an interesting point, a user could send a notification with a method like `generatetoaddress` and just not want to wait for a reply. There would be plenty of "getter" RPCs that would make no sense in a notification however and just waste time on the server. Maybe this is best suited for a follow-up
...
💬 achow101 commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#issuecomment-1654432367)
While I agree that doing something smart like this in the context of fee bumping is fine, `CreateTransaction` is a generic function for the wallet's transaction building so changing the behavior here will have an effect on other uses as well, so I don't think this is the right approach. In general, we shouldn't assume what the change is when it is ambiguous - conceivably a user could be reusing addresses or otherwise actually wants a particular change address that is also one of the outputs. Doi
...
(https://github.com/bitcoin/bitcoin/pull/27601#issuecomment-1654432367)
While I agree that doing something smart like this in the context of fee bumping is fine, `CreateTransaction` is a generic function for the wallet's transaction building so changing the behavior here will have an effect on other uses as well, so I don't think this is the right approach. In general, we shouldn't assume what the change is when it is ambiguous - conceivably a user could be reusing addresses or otherwise actually wants a particular change address that is also one of the outputs. Doi
...
💬 murchandamus commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#issuecomment-1654451888)
Perhaps this is a good way forward: only merge the new change output and the existing output on the original transaction if either a) Bitcoin Core organically detects that the original output was the change and it is also provided as the custom change address again, or b) if the user explicitly instructs the wallet to treat the original transaction’s output as the change per https://github.com/bitcoin/bitcoin/pull/26467 and provides the same address as custom change again. Otherwise, if the outp
...
(https://github.com/bitcoin/bitcoin/pull/27601#issuecomment-1654451888)
Perhaps this is a good way forward: only merge the new change output and the existing output on the original transaction if either a) Bitcoin Core organically detects that the original output was the change and it is also provided as the custom change address again, or b) if the user explicitly instructs the wallet to treat the original transaction’s output as the change per https://github.com/bitcoin/bitcoin/pull/26467 and provides the same address as custom change again. Otherwise, if the outp
...
💬 achow101 commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#issuecomment-1654461507)
ACK 26f91a56afd01ce11245944c66361da9aaa6a71e
(https://github.com/bitcoin/bitcoin/pull/28125#issuecomment-1654461507)
ACK 26f91a56afd01ce11245944c66361da9aaa6a71e
📝 jonatack opened a pull request: "rpc, util: avoid string copies in ParseHash/ParseHex functions"
(https://github.com/bitcoin/bitcoin/pull/28172)
These utility methods are called by quite a few RPCs and tests, as well as by each other.
```
$ git grep "ParseHashV\|ParseHashO\|ParseHexV\|ParseHexO" | wc -l
61
```
Also remove an out-of-date external link.
(https://github.com/bitcoin/bitcoin/pull/28172)
These utility methods are called by quite a few RPCs and tests, as well as by each other.
```
$ git grep "ParseHashV\|ParseHashO\|ParseHexV\|ParseHexO" | wc -l
61
```
Also remove an out-of-date external link.
💬 achow101 commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1276759711)
Perhaps check that this label doesn't appear in any of the migrated wallets?
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1276759711)
Perhaps check that this label doesn't appear in any of the migrated wallets?
💬 luke-jr commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1276765179)
Could just as well still be unsupported, not invalid.
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1276765179)
Could just as well still be unsupported, not invalid.
💬 jonatack commented on pull request "refactor: Revert additional univalue check in ParseSighashString":
(https://github.com/bitcoin/bitcoin/pull/28162#issuecomment-1654522671)
Tested ACK 06199a995f20c55583f6948cfe99e608679fcdf1
Verified relevant tests still pass after cherry-picking 647d95a from #28166 to this branch.
I don't mind re-ACKing if you like the suggestion in https://github.com/bitcoin/bitcoin/pull/28162#discussion_r1276034368 (mind the spaces on the last line of its diff or run clang-format on it).
(https://github.com/bitcoin/bitcoin/pull/28162#issuecomment-1654522671)
Tested ACK 06199a995f20c55583f6948cfe99e608679fcdf1
Verified relevant tests still pass after cherry-picking 647d95a from #28166 to this branch.
I don't mind re-ACKing if you like the suggestion in https://github.com/bitcoin/bitcoin/pull/28162#discussion_r1276034368 (mind the spaces on the last line of its diff or run clang-format on it).
💬 brunoerg commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1276789457)
Yea, just changed the approach to use buffer.
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1276789457)
Yea, just changed the approach to use buffer.
💬 brunoerg commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1276790725)
Yes, just changed it.
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1276790725)
Yes, just changed it.
💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1276818293)
I think it still makes sense because the functional test only covers the dns seed logic.
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1276818293)
I think it still makes sense because the functional test only covers the dns seed logic.
💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1276824800)
I think that usually it shouldn't substantially change the observations of a user, because 10 is a pretty high number and if all of those fail, we either have rally bad fixed seeds, or have a connectivity problem. I'll add a log entry with the next push though.
Also, I'll do some test runs to gather some statistics whether we might want to reduce this to 1 minute...
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1276824800)
I think that usually it shouldn't substantially change the observations of a user, because 10 is a pretty high number and if all of those fail, we either have rally bad fixed seeds, or have a connectivity problem. I'll add a log entry with the next push though.
Also, I'll do some test runs to gather some statistics whether we might want to reduce this to 1 minute...
📝 hebasto opened a pull request: "ci: Run Windows native task on GitHub Actions"
(https://github.com/bitcoin/bitcoin/pull/28173)
From https://github.com/bitcoin/bitcoin/issues/28098:
> Thus, someone would have to sponsor an amount of roughly 5kUSD/mo for those two tasks.
> If the goal is to stay on a free plan, I think the only option is GitHub Actions CI.
Historical context:
- https://github.com/bitcoin/bitcoin/pull/17697
- https://github.com/bitcoin/bitcoin/issues/17803
- https://github.com/bitcoin/bitcoin/pull/18031
Security concerns:
- https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-16514321
...
(https://github.com/bitcoin/bitcoin/pull/28173)
From https://github.com/bitcoin/bitcoin/issues/28098:
> Thus, someone would have to sponsor an amount of roughly 5kUSD/mo for those two tasks.
> If the goal is to stay on a free plan, I think the only option is GitHub Actions CI.
Historical context:
- https://github.com/bitcoin/bitcoin/pull/17697
- https://github.com/bitcoin/bitcoin/issues/17803
- https://github.com/bitcoin/bitcoin/pull/18031
Security concerns:
- https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-16514321
...
💬 luke-jr commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1276829144)
Prefer we just allow it for everything and error somewhere else
(https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1276829144)
Prefer we just allow it for everything and error somewhere else
💬 luke-jr commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1276830223)
ipc*
(https://github.com/bitcoin/bitcoin/pull/27679#discussion_r1276830223)
ipc*
🤔 furszy reviewed a pull request: "wallet: don't duplicate change output if already exist"
(https://github.com/bitcoin/bitcoin/pull/27601#pullrequestreview-1550813509)
> In general, we shouldn't assume what the change is when it is ambiguous - conceivably a user could be reusing addresses or otherwise actually wants a particular change address that is also one of the outputs. Doing this is allowed now, and I don't think we should change the meaning of the change address argument.
That is not really how the wallet behaves for regular outputs. Not sure why it should
behave differently for change outputs only.
Right now, the user cannot duplicate outputs i
...
(https://github.com/bitcoin/bitcoin/pull/27601#pullrequestreview-1550813509)
> In general, we shouldn't assume what the change is when it is ambiguous - conceivably a user could be reusing addresses or otherwise actually wants a particular change address that is also one of the outputs. Doing this is allowed now, and I don't think we should change the meaning of the change address argument.
That is not really how the wallet behaves for regular outputs. Not sure why it should
behave differently for change outputs only.
Right now, the user cannot duplicate outputs i
...
📝 aureleoules opened a pull request: "docs: Rewrite README to make it more appealing"
(https://github.com/bitcoin/bitcoin/pull/28174)
The current README is pretty bland and I believe it could need a few improvements to enhance its value and appeal.
This pull request updates the README to make it more appealing. The changes are the following:
* Adding emojis 🚀 which makes the document more vibrant
* Use a more casual and interactive language to engage the reader
All the original technical contents were preserved, and the structure of the document remains the same to ensure that users still find the information they're
...
(https://github.com/bitcoin/bitcoin/pull/28174)
The current README is pretty bland and I believe it could need a few improvements to enhance its value and appeal.
This pull request updates the README to make it more appealing. The changes are the following:
* Adding emojis 🚀 which makes the document more vibrant
* Use a more casual and interactive language to engage the reader
All the original technical contents were preserved, and the structure of the document remains the same to ensure that users still find the information they're
...