💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1649739156)
Cool, thanks! Added the fix and the test.
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1649739156)
Cool, thanks! Added the fix and the test.
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1649739250)
I disagree here. I think this is the correct use of an optional when we may or may not have a value to return. And I think it's easier to understand because the code is more expressive. Handing a magical `-1` around is just not intuitive IMO.
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1649739250)
I disagree here. I think this is the correct use of an optional when we may or may not have a value to return. And I think it's easier to understand because the code is more expressive. Handing a magical `-1` around is just not intuitive IMO.
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1649739281)
done
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1649739281)
done
💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#issuecomment-2184088934)
Addressed feedback from @stickies-v and @ryanofsky, thank you!
(https://github.com/bitcoin/bitcoin/pull/29668#issuecomment-2184088934)
Addressed feedback from @stickies-v and @ryanofsky, thank you!
👍 BrandonOdiwuor approved a pull request: "rest: don't copy block data when sending binary response"
(https://github.com/bitcoin/bitcoin/pull/30321#pullrequestreview-2133880309)
Code Review ACK 07770609afddaa1c730b431c207015f34b3a5f5f
(https://github.com/bitcoin/bitcoin/pull/30321#pullrequestreview-2133880309)
Code Review ACK 07770609afddaa1c730b431c207015f34b3a5f5f
📝 bombastictranz opened a pull request: "bombastictranz/bitcoin"
(https://github.com/bitcoin/bitcoin/pull/30323)
<!--
*** 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/30323)
<!--
*** 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
...
✅ bombastictranz closed a pull request: "bombastictranz/bitcoin"
(https://github.com/bitcoin/bitcoin/pull/30323)
(https://github.com/bitcoin/bitcoin/pull/30323)
💬 romanz commented on pull request "rest: don't copy block data when sending binary response":
(https://github.com/bitcoin/bitcoin/pull/30321#discussion_r1649767869)
Thanks for the suggestion!
What do you think about the following modification, in order to support `Span`?
```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 54fec59468..adc6040d08 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -634,7 +634,7 @@ void HTTPRequest::WriteHeader(const std::string& hdr, const std::string& value)
* Replies must be sent in the main loop in the main http thread,
* this cannot be done from worker threads.
*/
-void HTTPReques
...
(https://github.com/bitcoin/bitcoin/pull/30321#discussion_r1649767869)
Thanks for the suggestion!
What do you think about the following modification, in order to support `Span`?
```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 54fec59468..adc6040d08 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -634,7 +634,7 @@ void HTTPRequest::WriteHeader(const std::string& hdr, const std::string& value)
* Replies must be sent in the main loop in the main http thread,
* this cannot be done from worker threads.
*/
-void HTTPReques
...
🤔 TheCharlatan reviewed a pull request: "guix: build with glibc 2.31"
(https://github.com/bitcoin/bitcoin/pull/29987#pullrequestreview-2133927461)
Guix build (riscv64)
```
aa0ac90d4abc930dee2327985fafdbee9c40c8eeea5738383e798f076db36766 guix-build-b5fc6d46a385/output/aarch64-linux-gnu/SHA256SUMS.part
c25dec340e71ba13b238cc743cb86cb6ee564b05f47a1d3df2240f90b8cdf1cd guix-build-b5fc6d46a385/output/aarch64-linux-gnu/bitcoin-b5fc6d46a385-aarch64-linux-gnu-debug.tar.gz
b7e9627b89d06e2646fcc5430f2eb08ab920039cfa9d4ce14917830bc48ba129 guix-build-b5fc6d46a385/output/aarch64-linux-gnu/bitcoin-b5fc6d46a385-aarch64-linux-gnu.tar.gz
3afe6f97bc0
...
(https://github.com/bitcoin/bitcoin/pull/29987#pullrequestreview-2133927461)
Guix build (riscv64)
```
aa0ac90d4abc930dee2327985fafdbee9c40c8eeea5738383e798f076db36766 guix-build-b5fc6d46a385/output/aarch64-linux-gnu/SHA256SUMS.part
c25dec340e71ba13b238cc743cb86cb6ee564b05f47a1d3df2240f90b8cdf1cd guix-build-b5fc6d46a385/output/aarch64-linux-gnu/bitcoin-b5fc6d46a385-aarch64-linux-gnu-debug.tar.gz
b7e9627b89d06e2646fcc5430f2eb08ab920039cfa9d4ce14917830bc48ba129 guix-build-b5fc6d46a385/output/aarch64-linux-gnu/bitcoin-b5fc6d46a385-aarch64-linux-gnu.tar.gz
3afe6f97bc0
...
👍 TheCharlatan approved a pull request: "Enable clang-tidy checks for self-assignment"
(https://github.com/bitcoin/bitcoin/pull/30234#pullrequestreview-2133941453)
ACK 26a7f70b5d2b1cbfbf03e0b57b9b5ea92dafbb19
(https://github.com/bitcoin/bitcoin/pull/30234#pullrequestreview-2133941453)
ACK 26a7f70b5d2b1cbfbf03e0b57b9b5ea92dafbb19
💬 fjahr commented on issue "AssumeUTXO Mainnet Readiness Tracking":
(https://github.com/bitcoin/bitcoin/issues/29616#issuecomment-2184214743)
Moved up #29996 and added #30320 and #30288
(https://github.com/bitcoin/bitcoin/issues/29616#issuecomment-2184214743)
Moved up #29996 and added #30320 and #30288
👍 tdb3 approved a pull request: "rest: don't copy block data when sending binary response"
(https://github.com/bitcoin/bitcoin/pull/30321#pullrequestreview-2133999354)
ACK 07770609afddaa1c730b431c207015f34b3a5f5f
Simple and efficent. Looks good.
Tested with REST calls on two different nodes with signet block 201185 (one running PR 30321 and one running master (538363738e9e30813cf3e76ca4f71c1aaff349e7 at the time)).
PR 30321:
```
curl -v http://127.0.0.1:38332/rest/block/0000005d9cd746eeae8078c9bdb40866e02ddde50680c4c2d9d396dfdb6d3351.bin --output pr30321.bin
sha256sum pr30321.bin
0f88a3cda3b44b2f0fc66006f5ac1349cc8e6a2e7a70f7d5f756355c61d58c76
...
(https://github.com/bitcoin/bitcoin/pull/30321#pullrequestreview-2133999354)
ACK 07770609afddaa1c730b431c207015f34b3a5f5f
Simple and efficent. Looks good.
Tested with REST calls on two different nodes with signet block 201185 (one running PR 30321 and one running master (538363738e9e30813cf3e76ca4f71c1aaff349e7 at the time)).
PR 30321:
```
curl -v http://127.0.0.1:38332/rest/block/0000005d9cd746eeae8078c9bdb40866e02ddde50680c4c2d9d396dfdb6d3351.bin --output pr30321.bin
sha256sum pr30321.bin
0f88a3cda3b44b2f0fc66006f5ac1349cc8e6a2e7a70f7d5f756355c61d58c76
...
💬 iw4p commented on issue "Internal bug detected: Fee needed > fee paid":
(https://github.com/bitcoin/bitcoin/issues/29398#issuecomment-2184941436)
For some type of errors like this, why we're not containing the values (in this case fee_needed and current_fee) to show the users more clear error message (So he can realize faster what's wrong) and also easier for debugging?
(https://github.com/bitcoin/bitcoin/issues/29398#issuecomment-2184941436)
For some type of errors like this, why we're not containing the values (in this case fee_needed and current_fee) to show the users more clear error message (So he can realize faster what's wrong) and also easier for debugging?
🤔 rkrux requested changes to a pull request: "[test]: prevent `create_self_transfer` failure when target weight is below tx weight"
(https://github.com/bitcoin/bitcoin/pull/30322#pullrequestreview-2134242207)
tACK [d0a0408](https://github.com/bitcoin/bitcoin/pull/30322/commits/d0a0408fcfd3c184a25ca5330dc74f8ecd61f29f)
`make, test/functional` are successful.
Thanks @ismaelsadeeq for picking this up, I like the introduction of few assertions here. Added couple comment nits, and suggested one debug log change.
(https://github.com/bitcoin/bitcoin/pull/30322#pullrequestreview-2134242207)
tACK [d0a0408](https://github.com/bitcoin/bitcoin/pull/30322/commits/d0a0408fcfd3c184a25ca5330dc74f8ecd61f29f)
`make, test/functional` are successful.
Thanks @ismaelsadeeq for picking this up, I like the introduction of few assertions here. Added couple comment nits, and suggested one debug log change.
💬 rkrux commented on pull request "[test]: prevent `create_self_transfer` failure when target weight is below tx weight":
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1650043501)
The condition seems correct but the corresponding comment doesn't, it was a bit difficult for me to understand.
How about the following?
```
Prevent bulking tx when target weight (along with offset) is less than or equal to the tx weight
```
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1650043501)
The condition seems correct but the corresponding comment doesn't, it was a bit difficult for me to understand.
How about the following?
```
Prevent bulking tx when target weight (along with offset) is less than or equal to the tx weight
```
💬 rkrux commented on pull request "[test]: prevent `create_self_transfer` failure when target weight is below tx weight":
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1650044105)
Good addition imo.
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1650044105)
Good addition imo.
💬 rkrux commented on pull request "[test]: prevent `create_self_transfer` failure when target weight is below tx weight":
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1650043843)
Isn't this debug log incorrect?
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1650043843)
Isn't this debug log incorrect?
💬 rkrux commented on pull request "[test]: prevent `create_self_transfer` failure when target weight is below tx weight":
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1650041303)
Always nice to see getting rid of magic numbers!
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1650041303)
Always nice to see getting rid of magic numbers!
💬 rkrux commented on pull request "[test]: prevent `create_self_transfer` failure when target weight is below tx weight":
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1650043938)
```suggestion
self.log.debug(f"-> tx weight: {tx.get_weight()} is always greater than or equal target weight: {target_weight}")
```
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1650043938)
```suggestion
self.log.debug(f"-> tx weight: {tx.get_weight()} is always greater than or equal target weight: {target_weight}")
```
💬 rkrux commented on pull request "[test]: prevent `create_self_transfer` failure when target weight is below tx weight":
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1650041868)
➕
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1650041868)
➕