💬 theuni commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290640874)
Heh, we're getting there. Thanks for your patience!
This should get you one step further:
```patch
diff --git a/contrib/devtools/bitcoin-tidy/CMakeLists.txt b/contrib/devtools/bitcoin-tidy/CMakeLists.txt
index 9ed18696d4..ea74b98620 100644
--- a/contrib/devtools/bitcoin-tidy/CMakeLists.txt
+++ b/contrib/devtools/bitcoin-tidy/CMakeLists.txt
@@ -33,7 +33,7 @@ else()
target_compile_options(bitcoin-tidy PRIVATE -Wextra)
endif()
-set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "--loa
...
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290640874)
Heh, we're getting there. Thanks for your patience!
This should get you one step further:
```patch
diff --git a/contrib/devtools/bitcoin-tidy/CMakeLists.txt b/contrib/devtools/bitcoin-tidy/CMakeLists.txt
index 9ed18696d4..ea74b98620 100644
--- a/contrib/devtools/bitcoin-tidy/CMakeLists.txt
+++ b/contrib/devtools/bitcoin-tidy/CMakeLists.txt
@@ -33,7 +33,7 @@ else()
target_compile_options(bitcoin-tidy PRIVATE -Wextra)
endif()
-set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "--loa
...
🤔 jarolrod reviewed a pull request: "doc: Use GitHub's "Alert" markdown syntax"
(https://github.com/bitcoin/bitcoin/pull/28243#pullrequestreview-1572738308)
this would mess up the rendering in every markdown renderer except on Github, example:
<img width="634" alt="Screenshot 2023-08-10 at 4 07 03 PM" src="https://github.com/bitcoin/bitcoin/assets/23396902/a5e18b4d-9cfb-4a64-b24b-b8300aec4ff0">
(https://github.com/bitcoin/bitcoin/pull/28243#pullrequestreview-1572738308)
this would mess up the rendering in every markdown renderer except on Github, example:
<img width="634" alt="Screenshot 2023-08-10 at 4 07 03 PM" src="https://github.com/bitcoin/bitcoin/assets/23396902/a5e18b4d-9cfb-4a64-b24b-b8300aec4ff0">
💬 theuni commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290693717)
Assuming this is close to working, I've got a branch ready to PR here: https://github.com/theuni/bitcoin/tree/bitcoin-tidy-macos
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290693717)
Assuming this is close to working, I've got a branch ready to PR here: https://github.com/theuni/bitcoin/tree/bitcoin-tidy-macos
💬 Jones098 commented on issue "RPC `createmultisig` outputs a `sh(addr(...))` descriptor when address_type field is "p2sh-segwit"":
(https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1673945821)
Thanks for helping
(https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1673945821)
Thanks for helping
💬 jonatack commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290704000)
Happy to be learning by doing 😄
Using your branch, with the updated doc here:
```bash
jon|master =:~/bitcoin/bitcoin/contrib/devtools/bitcoin-tidy$ cmake -S . -B build
-DLLVM_DIR=$(llvm-config --cmakedir) -DCMAKE_BUILD_TYPE=Release
-- The C compiler identification is AppleClang 14.0.3.14030022
-- The CXX compiler identification is AppleClang 14.0.3.14030022
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /opt/homebrew/opt/c
...
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290704000)
Happy to be learning by doing 😄
Using your branch, with the updated doc here:
```bash
jon|master =:~/bitcoin/bitcoin/contrib/devtools/bitcoin-tidy$ cmake -S . -B build
-DLLVM_DIR=$(llvm-config --cmakedir) -DCMAKE_BUILD_TYPE=Release
-- The C compiler identification is AppleClang 14.0.3.14030022
-- The CXX compiler identification is AppleClang 14.0.3.14030022
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /opt/homebrew/opt/c
...
💬 Jones098 commented on issue "RPC `createmultisig` outputs a `sh(addr(...))` descriptor when address_type field is "p2sh-segwit"":
(https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1673949867)
> ### Is there an existing issue for this?
>
> - [X] I have searched the existing issues
>
> ### Current behaviour
>
> While executing rpc createmultisig it outputs a addr descriptor wrapped around sh. Addr descriptor is supposed to be a top level descriptor. Please refer to [this gist](https://gist.github.com/Vasu-08/57c1ff479baf2e70e7e2195d31575651) for further details.
>
> ### Expected behaviour
>
> Not sure.
> true
> ### Steps to reproduce
> yes
> 
> ### Is there an existing issue for this?
>
> - [X] I have searched the existing issues
>
> ### Current behaviour
>
> While executing rpc createmultisig it outputs a addr descriptor wrapped around sh. Addr descriptor is supposed to be a top level descriptor. Please refer to [this gist](https://gist.github.com/Vasu-08/57c1ff479baf2e70e7e2195d31575651) for further details.
>
> ### Expected behaviour
>
> Not sure.
> true
> ### Steps to reproduce
> yes
> ![image](https://github.com/bitcoin
...
💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1673956144)
> > All of those transactions are taken from my OpenTimestamps calendars. My OTS calendars bump fees repeatedly, increasing the feerate by 1sat/vb with each replacement, starting at approximately the minimum relay fee.
>
> Yes, that is a pretty perfect mechanism if you want to create a false positive for "full rbf" when all you're actually triggering is eviction from the mempool due to a change in minfee. For nodes that don't run "full rbf", they'll see one of the early transactions, add it t
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1673956144)
> > All of those transactions are taken from my OpenTimestamps calendars. My OTS calendars bump fees repeatedly, increasing the feerate by 1sat/vb with each replacement, starting at approximately the minimum relay fee.
>
> Yes, that is a pretty perfect mechanism if you want to create a false positive for "full rbf" when all you're actually triggering is eviction from the mempool due to a change in minfee. For nodes that don't run "full rbf", they'll see one of the early transactions, add it t
...
💬 jonatack commented on pull request "p2p: outbound network diversity refactoring and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1673966621)
@dergoegge updated
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1673966621)
@dergoegge updated
📝 furszy opened a pull request: "test: display abrupt shutdown errors in console output"
(https://github.com/bitcoin/bitcoin/pull/28253)
Making it easier to debug errors in the CI environment,
particularly in scenarios where it's not immediately clear
what happened nor which node crashed (or shutdown abruptly).
A bit of context:
Currently, the test framework redirects each node's stderr output
stream to a different temporary file inside each node's data directory.
While this is sufficient for storing the error, it isn't very helpful for
understanding what happened just by reading the CI console output.
Most of the tim
...
(https://github.com/bitcoin/bitcoin/pull/28253)
Making it easier to debug errors in the CI environment,
particularly in scenarios where it's not immediately clear
what happened nor which node crashed (or shutdown abruptly).
A bit of context:
Currently, the test framework redirects each node's stderr output
stream to a different temporary file inside each node's data directory.
While this is sufficient for storing the error, it isn't very helpful for
understanding what happened just by reading the CI console output.
Most of the tim
...
💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1290699614)
nit: This phrasing seems to suggest that the developer is free to choose which `Arg` call to use. I would change that to be more explicit, along the lines of (I think this can be improved):
```
If the argument has a `RPCArg::Default` specified, you must use `Arg<Type>` to get the argument or its default value.
Otherwise, you must use Arg<Type*> to get a pointer or std::optional to get the argument, or a falsy value if it was not provided. A pointer is returned if the underlying is a referen
...
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1290699614)
nit: This phrasing seems to suggest that the developer is free to choose which `Arg` call to use. I would change that to be more explicit, along the lines of (I think this can be improved):
```
If the argument has a `RPCArg::Default` specified, you must use `Arg<Type>` to get the argument or its default value.
Otherwise, you must use Arg<Type*> to get a pointer or std::optional to get the argument, or a falsy value if it was not provided. A pointer is returned if the underlying is a referen
...
💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1290695286)
Since we're calling `std::get<RPCArg::Default>(fallback);` later on, wouldn't it make more sense to call `if (has_default) CHECK_NONFATAL(std::holds_alternative<RPCArg::Default>(fallback));`?
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1290695286)
Since we're calling `std::get<RPCArg::Default>(fallback);` later on, wouldn't it make more sense to call `if (has_default) CHECK_NONFATAL(std::holds_alternative<RPCArg::Default>(fallback));`?
💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1290694437)
nit: I find affirmative names much easier to understand than negative ones. Event though there's no double negation used here (but it would in another suggestion I made), would you consider changing this to `has_default`? I find that much clearer on the template instantiation calls too.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1290694437)
nit: I find affirmative names much easier to understand than negative ones. Event though there's no double negation used here (but it would in another suggestion I made), would you consider changing this to `has_default`? I find that much clearer on the template instantiation calls too.
💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1290703491)
We could remove `no_default` from the function signature here, and deduce it from the `ret_type` being a std::optional or pointer type? I think both approaches have merit, so no very strong preference either way atm, but since we're already doing those deductions in `Arg` I think it would make sense here, too?
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1290703491)
We could remove `no_default` from the function signature here, and deduce it from the `ret_type` being a std::optional or pointer type? I think both approaches have merit, so no very strong preference either way atm, but since we're already doing those deductions in `Arg` I think it would make sense here, too?
💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1290725116)
> But this also may be more confusing than it helping.
Yeah, I don't really think that helps. Still no strong view, I like your current implementation, it's less complex than what I suggested, so barring strong conviction that's good enough reason for me to keep it as is. Perhaps useful to keep this convo open for future reference, but considered resolved from my end.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1290725116)
> But this also may be more confusing than it helping.
Yeah, I don't really think that helps. Still no strong view, I like your current implementation, it's less complex than what I suggested, so barring strong conviction that's good enough reason for me to keep it as is. Perhaps useful to keep this convo open for future reference, but considered resolved from my end.
👍 stickies-v approved a pull request: "rpc: Add Arg() default helper"
(https://github.com/bitcoin/bitcoin/pull/28230#pullrequestreview-1572794322)
Thanks for extending this to cover all kinds of argument types, I think this will make it much more straightforward for users to add in their own template instantiations where needed.
I still [don't really have a clear preference](https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1287432683) for std::optional vs pointer, and I think it's not that important, so ACK fa56ad74160ba3c07a16a661fc14cded6deed9eb
(https://github.com/bitcoin/bitcoin/pull/28230#pullrequestreview-1572794322)
Thanks for extending this to cover all kinds of argument types, I think this will make it much more straightforward for users to add in their own template instantiations where needed.
I still [don't really have a clear preference](https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1287432683) for std::optional vs pointer, and I think it's not that important, so ACK fa56ad74160ba3c07a16a661fc14cded6deed9eb
🤔 jonatack reviewed a pull request: "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain"
(https://github.com/bitcoin/bitcoin/pull/28157#pullrequestreview-1572801253)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28157#pullrequestreview-1572801253)
Concept ACK
💬 jonatack commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1290730731)
I'm not sure why, and maybe it's an issue with my local environment as the CI is green, but this test passes for me in the previous push 9bd96327232ac9fbb7f9cb0a0545a4410bdd4964 but not with the latest push ef19d52e25407df1a56a76038dd4c95e03e75250; only the signet case passes while the main and test cases raise the following:
```
AssertionError: [node 0] Expected message "Error: acceptstalefeeestimates is not supported on test chain." does not fully match stderr:
"Error: Unable to start HTT
...
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1290730731)
I'm not sure why, and maybe it's an issue with my local environment as the CI is green, but this test passes for me in the previous push 9bd96327232ac9fbb7f9cb0a0545a4410bdd4964 but not with the latest push ef19d52e25407df1a56a76038dd4c95e03e75250; only the signet case passes while the main and test cases raise the following:
```
AssertionError: [node 0] Expected message "Error: acceptstalefeeestimates is not supported on test chain." does not fully match stderr:
"Error: Unable to start HTT
...
💬 jonatack commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1290746042)
It turns out there isn't a difference between the two pushes. The issue comes from having bitcoind already running on mainnet, testnet, or signet; if yes, the test fails with the message I described.
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1290746042)
It turns out there isn't a difference between the two pushes. The issue comes from having bitcoind already running on mainnet, testnet, or signet; if yes, the test fails with the message I described.
💬 theuni commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290765007)
Woohoo! I'll PR that commit.
Thanks again for testing, glad to see it's working on macOS (and arm64 even).
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290765007)
Woohoo! I'll PR that commit.
Thanks again for testing, glad to see it's working on macOS (and arm64 even).
💬 ajtowns commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1290764018)
Contrary to its comment, this is only used when validating mempool transactions, so is better placed in policy.h
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1290764018)
Contrary to its comment, this is only used when validating mempool transactions, so is better placed in policy.h