💬 1440000bytes commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1164567117)
Is there a reason for this restriction?
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1164567117)
Is there a reason for this restriction?
💬 achow101 commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1164574104)
IIRC updating this RPC would require pretty significant changes to it that felt out of scope for this PR. I've left it as something to do in a followup in order to keep the scope of the PR limited to just the bare minimum to work with PSBTv2.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1164574104)
IIRC updating this RPC would require pretty significant changes to it that felt out of scope for this PR. I've left it as something to do in a followup in order to keep the scope of the PR limited to just the bare minimum to work with PSBTv2.
💬 pinheadmz commented on issue "Write instructions on offline signing.":
(https://github.com/bitcoin/bitcoin/issues/9492#issuecomment-1505838532)
https://github.com/bitcoin/bitcoin/blob/master/doc/external-signer.md was added by @Sjors over 2 years ago. I also found some good answers about `signrawtransaction` on stack exchange. There is also https://github.com/bitcoin/bitcoin/blob/master/doc/psbt.md which is about as old. I think PSBT was added to Bitcoin after this issue was opened and although the guides are more geared to multisig, they cover a lot.
@adamjonas do you think those docs are good enough to close this issue? Otherwise I
...
(https://github.com/bitcoin/bitcoin/issues/9492#issuecomment-1505838532)
https://github.com/bitcoin/bitcoin/blob/master/doc/external-signer.md was added by @Sjors over 2 years ago. I also found some good answers about `signrawtransaction` on stack exchange. There is also https://github.com/bitcoin/bitcoin/blob/master/doc/psbt.md which is about as old. I think PSBT was added to Bitcoin after this issue was opened and although the guides are more geared to multisig, they cover a lot.
@adamjonas do you think those docs are good enough to close this issue? Otherwise I
...
💬 pinheadmz commented on issue "Alerting blockchain reorgs":
(https://github.com/bitcoin/bitcoin/issues/14915#issuecomment-1505843460)
NACK, I don't think there is a really great use for this for most users. Reorgs are currently very rare and when they happen they are 1 block at most. The GUI should and does show users if their transactions are unconfirmed, that should be the most important thing. As for (b) an advanced user can use `-blocknotify` and some external script to detect that more than one block at the same height has been processed.
(https://github.com/bitcoin/bitcoin/issues/14915#issuecomment-1505843460)
NACK, I don't think there is a really great use for this for most users. Reorgs are currently very rare and when they happen they are 1 block at most. The GUI should and does show users if their transactions are unconfirmed, that should be the most important thing. As for (b) an advanced user can use `-blocknotify` and some external script to detect that more than one block at the same height has been processed.
💬 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_r1164585860)
> Well I meant something like a switch statement with `"-debug"` and `"-debugexclude"` literal strings as cases.
The condition in a [switch](https://en.cppreference.com/w/cpp/language/switch) statement must be of integer or enum type, or of a class type implicitly convertible to one.
But we can still use conditionals. Do you like this better?
```diff
static constexpr std::array DEBUG_LOG_OPTS{"-debug", "-debugexclude"};
+static bool EnableOrDisable(const std::string& opt, const st
...
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1164585860)
> Well I meant something like a switch statement with `"-debug"` and `"-debugexclude"` literal strings as cases.
The condition in a [switch](https://en.cppreference.com/w/cpp/language/switch) statement must be of integer or enum type, or of a class type implicitly convertible to one.
But we can still use conditionals. Do you like this better?
```diff
static constexpr std::array DEBUG_LOG_OPTS{"-debug", "-debugexclude"};
+static bool EnableOrDisable(const std::string& opt, const st
...
💬 willcl-ark commented on issue "doc: Consider installing doc/ with make install":
(https://github.com/bitcoin/bitcoin/issues/16897#issuecomment-1505850816)
Is this something that we actually want to add? IMO adding a load of (mostly) markdown files to this dir would be clunky at best...
If we don't plan for this then I think we can close the issue.
If we do then it would probably make sense to wait until the cmake project has resolved one way or another before making (low priority) changes to the make process?
(https://github.com/bitcoin/bitcoin/issues/16897#issuecomment-1505850816)
Is this something that we actually want to add? IMO adding a load of (mostly) markdown files to this dir would be clunky at best...
If we don't plan for this then I think we can close the issue.
If we do then it would probably make sense to wait until the cmake project has resolved one way or another before making (low priority) changes to the make process?
💬 pinheadmz 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_r1164601290)
Yeah 😬 waddaya think?
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1164601290)
Yeah 😬 waddaya think?
💬 ismaelsadeeq commented on pull request "test: add coverage to rpc_scantxoutset.py":
(https://github.com/bitcoin/bitcoin/pull/27422#issuecomment-1505870199)
Thank you for picking that up.
I will take a look at that.
(https://github.com/bitcoin/bitcoin/pull/27422#issuecomment-1505870199)
Thank you for picking that up.
I will take a look at that.
💬 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_r1164606990)
Sure, will update. And add the comments mentioned in https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1164530898.
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1164606990)
Sure, will update. And add the comments mentioned in https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1164530898.
💬 ismaelsadeeq commented on pull request "test: added coverage to rpc_scantxoutset.py":
(https://github.com/bitcoin/bitcoin/pull/27453#issuecomment-1505878687)
LGTM Ack ecb79aed4d847d8c95936ac80b7e137f9c17b6f8
(https://github.com/bitcoin/bitcoin/pull/27453#issuecomment-1505878687)
LGTM Ack ecb79aed4d847d8c95936ac80b7e137f9c17b6f8
🤔 furszy reviewed a pull request: "wallet: when a block is disconnected, update transactions that are no longer conflicted"
(https://github.com/bitcoin/bitcoin/pull/27145#pullrequestreview-1380282608)
Haven't finished checking the latest changes but, if you like it, https://github.com/furszy/bitcoin-core/commit/8bb14c1eaf09b0bca9efa78e1a7a7a00c61c0a29 is all yours. During the review, the functional test wasn't clear enough for me so I spent a bit of time making it more friendly.
(https://github.com/bitcoin/bitcoin/pull/27145#pullrequestreview-1380282608)
Haven't finished checking the latest changes but, if you like it, https://github.com/furszy/bitcoin-core/commit/8bb14c1eaf09b0bca9efa78e1a7a7a00c61c0a29 is all yours. During the review, the functional test wasn't clear enough for me so I spent a bit of time making it more friendly.
💬 furszy commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163469554)
I would try to not provide the context by reference to the function. The lambda function's `wtx` shadows the function's `wtx`. Could just provide `disconnect_height`.
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1163469554)
I would try to not provide the context by reference to the function. The lambda function's `wtx` shadows the function's `wtx`. Could just provide `disconnect_height`.
💬 rsafier commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1505973365)
I made some quick mods to have working docker for this PR: https://github.com/rsafier/bitcoin_signet/tree/carman-blocktime-adjustable
I used it to get my 10Msats.
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1505973365)
I made some quick mods to have working docker for this PR: https://github.com/rsafier/bitcoin_signet/tree/carman-blocktime-adjustable
I used it to get my 10Msats.
💬 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_r1164674497)
Done, and also dropped `DEBUG_LOG_OPTS` and the no-longer useful `Assume` check (the `NONFATAL_UNREACHABLE` check does it better), and made the two `EnableOrDisableLogCategories` functions simpler and more similar. Very helpful feedback (thanks!)
<details><summary><code>git diff e9f6fd0 8ac6539</code></summary><p>
```diff
diff --git a/src/init/common.cpp b/src/init/common.cpp
index a5b07420d94..c99a512459b 100644
--- a/src/init/common.cpp
+++ b/src/init/common.cpp
@@ -77,26 +77,28 @@
...
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1164674497)
Done, and also dropped `DEBUG_LOG_OPTS` and the no-longer useful `Assume` check (the `NONFATAL_UNREACHABLE` check does it better), and made the two `EnableOrDisableLogCategories` functions simpler and more similar. Very helpful feedback (thanks!)
<details><summary><code>git diff e9f6fd0 8ac6539</code></summary><p>
```diff
diff --git a/src/init/common.cpp b/src/init/common.cpp
index a5b07420d94..c99a512459b 100644
--- a/src/init/common.cpp
+++ b/src/init/common.cpp
@@ -77,26 +77,28 @@
...
💬 jonatack commented on pull request "doc: remove incorrect line from example":
(https://github.com/bitcoin/bitcoin/pull/27454#discussion_r1164694164)
Instead of removing, maybe clarify along the lines of
`Create a transaction with no inputs specified (automatically include coins from the wallet)`
(https://github.com/bitcoin/bitcoin/pull/27454#discussion_r1164694164)
Instead of removing, maybe clarify along the lines of
`Create a transaction with no inputs specified (automatically include coins from the wallet)`
💬 jonatack commented on pull request "doc: remove incorrect line from example":
(https://github.com/bitcoin/bitcoin/pull/27454#discussion_r1164695637)
(While here, perhaps add more examples to the help, if that seems like it would be useful.)
(https://github.com/bitcoin/bitcoin/pull/27454#discussion_r1164695637)
(While here, perhaps add more examples to the help, if that seems like it would be useful.)
📝 Muhammedhamid23 opened a pull request: "Muhammedhamid23 patch 1"
(https://github.com/bitcoin/bitcoin/pull/27457)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/27457)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
✅ achow101 closed a pull request: "Muhammedhamid23 patch 1"
(https://github.com/bitcoin/bitcoin/pull/27457)
(https://github.com/bitcoin/bitcoin/pull/27457)
📝 achow101 locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/27457)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/27457)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
🤔 ajtowns reviewed a pull request: "addrman: Enable selecting addresses by network"
(https://github.com/bitcoin/bitcoin/pull/27214#pullrequestreview-1382390744)
ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6
(https://github.com/bitcoin/bitcoin/pull/27214#pullrequestreview-1382390744)
ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6